[squeak-dev] The Inbox: Monticello-ul.727.mcz

Levente Uzonyi leves at caesar.elte.hu
Thu Sep 17 10:49:28 UTC 2020


Hi Christoph,

On Thu, 17 Sep 2020, Thiede, Christoph wrote:

> 
> Very nice idea! :-) If I wanted to hijack an image, these URLRewriteRules would probably be my first approach - but security has never been an issue for Squeak, so I guess this is not a problem.

If you can manipulate objects like URLRewriteRules, you have already 
hijacked the image.

> 
> 
> Does this also fix the problem with the classic HTTP URLs returned by the update map or will we still need to patch them on the server side?

The urls in the update map are used to create repositories. That's why 
simply changing the existing http repository urls to https doesn't 
suffice, because the updater will see the http urls and create new 
repositores with them if they are absent.

So, this rewrite trick of Eliot works around the problem of the update 
maps as well.

> 
> 
> And one last question regarding to your tests in the method comment of #rewriteUrl:forDownload:: Couldn't you put them into a real test case? I'm pretty sure that not everyone will run these out-commented tests manually, and
> it would be a pity not to automate them.

I left the possibility to change the rewrite rules to whatever you want. 
If there were a test case with those asserts in the comment, the test 
would start failing as soon as you changed the rules.
Thought it's possible to create a test case which temporarily resets to 
the default rules and tests them.


Levente

> 
> 
> Best,
> 
> Christoph
> 
> _________________________________________________________________________________________________________________________________________________________________________________________________________________________________
> Von: Squeak-dev <squeak-dev-bounces at lists.squeakfoundation.org> im Auftrag von commits at source.squeak.org <commits at source.squeak.org>
> Gesendet: Donnerstag, 17. September 2020 01:55:41
> An: squeak-dev at lists.squeakfoundation.org
> Betreff: [squeak-dev] The Inbox: Monticello-ul.727.mcz  
> Levente Uzonyi uploaded a new version of Monticello to project The Inbox:
> http://source.squeak.org/inbox/Monticello-ul.727.mcz
> 
> ==================== Summary ====================
> 
> Name: Monticello-ul.727
> Author: ul
> Time: 17 September 2020, 1:54:51.056164 am
> UUID: ad776836-42eb-4aa2-b788-f10dd9e07da2
> Ancestors: Monticello-cmm.726
> 
> MCHttpRepository changes:
> - before up- or downloading files, transform the urls using #rewriteUrl:forDownload:. The default rules (see #urlRewriteRules) switch from http to https for source.squeak.org and squeaksource.com, and switch to the the static
> smalltalkhub site for downloads. The url rewriting is Eliot's idea, but this implementation uses a list of rewrite rules instead of a dictionary-based mapping.
> - use WebClient (and the shared webclient instance) for uploads too
> - retry down/uploading with WebClient at most 3 times. This should work around the case where the underlying socket was closed but the state of the socket has not been updated in Squeak.
> - use https in #creationTemplate
> 
> =============== Diff against Monticello-cmm.726 ===============
> 
> Item was changed:
>   MCFileBasedRepository subclass: #MCHttpRepository
>          instanceVariableNames: 'location user password readerCache indexed webClient'
> +        classVariableNames: 'URLRewriteRules UseSharedWebClientInstance'
> -        classVariableNames: 'UseSharedWebClientInstance'
>          poolDictionaries: ''
>          category: 'Monticello-Repositories'!
> 
> Item was changed:
>   ----- Method: MCHttpRepository class>>creationTemplate (in category 'ui-support') -----
>   creationTemplate
> +        ^self creationTemplateLocation: 'https://www.squeaksource.com/ProjectName'
> -        ^self creationTemplateLocation: 'http://www.squeaksource.com/ProjectName'
>                  user: 'squeak'
>                  password: 'squeak'
>   !
> 
> Item was added:
> + ----- Method: MCHttpRepository class>>rewriteUrl:forDownload: (in category 'url rewrite') -----
> + rewriteUrl: aString forDownload: forDownload
> +
> +        | result |
> +        result := aString.
> +        self urlRewriteRules groupsDo: [ :regexString :replacement :downloadOnly |
> +                (forDownload or: [ downloadOnly not ])  ifTrue: [
> +                        result := result copyWithRegex: regexString matchesReplacedWith: replacement ] ].
> +        ^result
> +       
> + "
> + self assert:  'https://squeaksource.com/foo/bar?baz=1' = (self rewriteUrl: 'http://squeaksource.com/foo/bar?baz=1' forDownload: true).
> + self assert:  'https://squeaksource.com/foo/bar?baz=1' = (self rewriteUrl: 'https://squeaksource.com/foo/bar?baz=1' forDownload: true).
> + self assert:  'https://source.squeak.org/foo/bar?baz=1' = (self rewriteUrl: 'http://source.squeak.org/foo/bar?baz=1' forDownload: true).
> + self assert:  'https://source.squeak.org/foo/bar?baz=1' = (self rewriteUrl: 'https://source.squeak.org/foo/bar?baz=1' forDownload: true).
> + self assert:  'http://static.smalltalkhub.com/foo/bar?baz=1' = (self rewriteUrl: 'http://smalltalkhub.com/foo/bar?baz=1' forDownload: true).
> + self assert:  'http://smalltalkhub.com/foo/bar?baz=1' = (self rewriteUrl: 'http://smalltalkhub.com/foo/bar?baz=1' forDownload: false).
> + "!
> 
> Item was added:
> + ----- Method: MCHttpRepository class>>urlRewriteRules (in category 'url rewrite') -----
> + urlRewriteRules
> +
> +        ^URLRewriteRules ifNil: [
> +                URLRewriteRules := #(
> +                        "Regex to be replaced"  "static replacement string"     "download only"
> +                        '^http\://source\.squeak\.org/' 'https://source.squeak.org/' false
> +                        '^http\://squeaksource\.com/' 'https://squeaksource.com/' false
> +                        '^http\://www.squeaksource\.com/' 'https://www.squeaksource.com/' false
> +                        '^http\://smalltalkhub.com/' 'http://static.smalltalkhub.com/' true    
> +                )  asOrderedCollection ]!
> 
> Item was changed:
>   ----- Method: MCHttpRepository>>httpGet:arguments: (in category 'private') -----
>   httpGet: url arguments: arguments
>  
> +        | urlString |
> -        | progress urlString client  response result |
> -        progress := [ :total :amount |
> -                HTTPProgress new
> -                        total: total;
> -                        amount: amount;
> -                        signal: 'Downloading...' ].
>          urlString := arguments
>                  ifNil: [ url ]
>                  ifNotNil: [
>                          | queryString |
>                          queryString := WebUtils encodeUrlEncodedForm: arguments.
>                          (url includes: $?)
>                                  ifTrue: [ url, '&', queryString ]
>                                  ifFalse: [ url, '?', queryString ] ].
> +        urlString := self class rewriteUrl: urlString forDownload: true.
> +        ^self webClientDo: [ :client |
> +                client
> +                        username: self user;
> +                        password: self password;
> +                        httpGet: urlString do: [ :request |
> +                                request
> +                                        headerAt: 'Authorization' put: 'Basic ', (self user, ':', self password) base64Encoded;
> +                                        headerAt: 'Connection' put: 'Keep-Alive';
> +                                        headerAt: 'Accept' put: '*/*' ] ]!
> -        self class useSharedWebClientInstance ifTrue: [
> -                "Acquire webClient by atomically storing it in the client variable and setting its value to nil."
> -                client := webClient.
> -                webClient := nil ].
> -        client
> -                ifNil: [ client := WebClient new ]
> -                ifNotNil: [
> -                        "Attempt to avoid an error on windows by recreating the underlying stream."
> -                        client isConnected ifFalse: [ client close ] ].
> -        response := client
> -                username: self user;
> -                password: self password;
> -                httpGet: urlString do: [ :request |
> -                        request
> -                                headerAt: 'Authorization' put: 'Basic ', (self user, ':', self password) base64Encoded;
> -                                headerAt: 'Connection' put: 'Keep-Alive';
> -                                headerAt: 'Accept' put: '*/*' ].
> -        result := (response code between: 200 and: 299)
> -                ifFalse: [
> -                        response content. "Make sure content is read."
> -                        nil ]
> -                ifTrue: [ (RWBinaryOrTextStream with: (response contentWithProgress: progress)) reset ].
> -        self class useSharedWebClientInstance
> -                ifTrue: [
> -                        "Save the WebClient instance for reuse, but only if there is no client cached."
> -                        webClient 
> -                                ifNil: [ webClient := client ]
> -                                ifNotNil: [ client close ] ]
> -                ifFalse: [ client close ].
> -        result ifNil: [ NetworkError signal: 'Could not access ', location ].
> -        ^result!
> 
> Item was added:
> + ----- Method: MCHttpRepository>>webClientDo: (in category 'private') -----
> + webClientDo: aBlock
> +
> +        | client attemptsLeft response result |
> +        self class useSharedWebClientInstance ifTrue: [
> +                "Acquire webClient by atomically storing it in the client variable and setting its value to nil."
> +                client := webClient.
> +                webClient := nil ].
> +       
> +        client
> +                ifNil: [ client := WebClient new ]
> +                ifNotNil: [
> +                        "Attempt to avoid an error by recreating the underlying stream."
> +                        client isConnected ifFalse: [ client close ] ].
> +               
> +        attemptsLeft := 3.
> +        response := nil.
> +        [ response isNil and: [ attemptsLeft > 0 ] ] whileTrue: [
> +                response := [ aBlock value: client ]
> +                        on: NetworkError
> +                        do: [ :error |
> +                                attemptsLeft = 0 ifTrue: [ error pass ].
> +                                (3 - attemptsLeft) seconds asDelay wait.
> +                                attemptsLeft := attemptsLeft - 1.
> +                                nil "The response" ] ].
> +       
> +        result := (response code between: 200 and: 299)
> +                ifFalse: [
> +                        response content. "Make sure content is read."
> +                        nil ]
> +                ifTrue: [
> +                        (RWBinaryOrTextStream with: (
> +                                response contentWithProgress:  [ :total :amount |
> +                                        HTTPProgress new
> +                                                total: total;
> +                                                amount: amount;
> +                                                signal ])) reset ].
> +
> +        self class useSharedWebClientInstance
> +                ifTrue: [
> +                        "Save the WebClient instance for reuse, but only if there is no client cached."
> +                        webClient 
> +                                ifNil: [ webClient := client ]
> +                                ifNotNil: [ client close ] ]
> +                ifFalse: [ client close ].
> +
> +        result ifNil: [ NetworkError signal: 'Could not access ', location ].
> +        ^result!
> 
> Item was changed:
>   ----- Method: MCHttpRepository>>writeStreamForFileNamed:replace:do: (in category 'private') -----
>   writeStreamForFileNamed: aString replace: ignoreBoolean do: aBlock
> +
> +        | stream urlString |
> -        | stream response statusLine code |
>          stream := RWBinaryOrTextStream on: String new.
>          aBlock value: stream.
> +        urlString := self urlForFileNamed: aString.
> +        urlString := self class rewriteUrl: urlString forDownload: false.
> +        ^self displayProgress: 'Uploading ', aString during: [
> +                self webClientDo: [ :client |
> +                        client
> +                                username: self user;
> +                                password: self password;
> +                                httpPut: urlString
> +                                        content: stream contents
> +                                        type: nil
> +                                        do: [ :request |
> +                                                request
> +                                                        headerAt: 'Authorization' put: 'Basic ', (self user, ':', self password) base64Encoded;
> +                                                        headerAt: 'Connection' put: 'Keep-Alive';
> +                                                        headerAt: 'Accept' put: '*/*'  ] ] ]!
> -        self displayProgress: 'Uploading ', aString during:[
> -                response := HTTPSocket
> -                                        httpPut: stream contents
> -                                        to: (self urlForFileNamed: aString)
> -                                        user: self user
> -                                        passwd: self password.
> -        ].
> -        "More robust handling of HTTP responses. Instead of enumerating
> -        all possible return codes and http versions, do a quick parse"
> -        (response beginsWith: 'HTTP/') ifTrue:[
> -                "Looks like an HTTP header, not some error message"
> -                statusLine := response copyUpTo: Character cr.
> -                code := [(statusLine findTokens: ' ') second asInteger] on: Error do:[].
> -        ].
> -        (code isInteger and:[code between: 200 and: 299])
> -                ifFalse:[self error: response].!
> 
> 
> 
>


More information about the Squeak-dev mailing list