Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug in WAAbstractFileLibrary #786

Closed
GoogleCodeExporter opened this issue Mar 25, 2015 · 5 comments
Closed

Bug in WAAbstractFileLibrary #786

GoogleCodeExporter opened this issue Mar 25, 2015 · 5 comments

Comments

@GoogleCodeExporter
Copy link

Hi there,

over the last few nights, our Seaside Application was bombarded with requests 
that were formed like this:

/files/JQUiDeploymentLibrary/%29.find%28

The attacks did also try other javascript expressions.

Unfortunately, WAAbstractFileLibrary reacts to this by throwing a primitive 
failed on VA Smalltalk in WAAbstractFileLibrary class>>#asSelector:, because 
the javascript expression cannot be interpreted as a filename.

Here's an excerpt of our walkback that shows what's going on.

String(Object)>>#primitiveFailed
  receiver = ''
String>>#at:
  receiver = ''
  arg1 = 1
String(SequenceableCollection)>>#first
  receiver = ''
JQUiDeploymentLibrary class(WAAbstractFileLibrary class)>>#asSelector:
  receiver = JQUiDeploymentLibrary
  arg1 = ').find('
  temp1 = ''
  temp2 = nil
JQUiDeploymentLibrary(WAAbstractFileLibrary)>>#asSelector:
  receiver = a JQUiDeploymentLibrary
  arg1 = ').find('
JQUiDeploymentLibrary(WAFileLibrary)>>#handle:
  receiver = a JQUiDeploymentLibrary
  arg1 = a WARequestContext url: '/files/JQUiDeploymentLibrary/%29.find%28'
  temp1 = ').find('
  temp2 = nil
  temp3 = nil
JQUiDeploymentLibrary class(WAAbstractFileLibrary class)>>#handle:
  receiver = JQUiDeploymentLibrary
  arg1 = a WARequestContext url: '/files/JQUiDeploymentLibrary/%29.find%28'

I am on the road and have no pharo/seaside image with me, but if I remember 
correctly, pharo does not throw an exception when you ask an empty string for 
its #first character, I seem to remember it just returns nil. VA Smalltalk does 
throw an exception. It does not stop working, so this is not a critical problem.

However, I think an additional check in #asSelector: wouldn't hurt because then 
the result is an http error code 404, which can either be returned to the 
client or removed by filters like mod_security.

So here is a fix for WAAbstractFileLibrary class>>asSelector: that I suggest 
for inclusion in Seaside, even if it is unnecessary for Pharo:

asSelector: aFilename
    | mainPart extension |
    mainPart := (aFilename copyUpToLast: $.)
        select: [ :each | each isAlphaNumeric ].

    mainPart isEmptyOrNil ifTrue: [^nil].

    [ mainPart first isDigit ]
        whileTrue: [ mainPart := mainPart allButFirst ].
    extension := (aFilename copyAfterLast: $.) asLowercase capitalized.
    ^ (mainPart, extension) asSymbol

Joachim

Original issue reported on code.google.com by philippe...@gmail.com on 14 Apr 2014 at 7:51

@GoogleCodeExporter
Copy link
Author

I actually think there are two problems here:


1) Sending first to an empty collection is not portable (ANSI says the results 
are undefined). This is the problem addressed by Joachim's suggested change.
2) $( (close parenthesis), which is provided as part of the extension, is not a 
valid selector character.

I believe the correct method is:

asSelector: aFilename
    | mainPart extension |
    mainPart := (aFilename copyUpToLast: $.)
        select: [ :each | each isAlphaNumeric ].

    mainPart isEmptyOrNil ifTrue: [ ^ nil ].

    [ mainPart first isDigit ]
        whileTrue: [ mainPart := mainPart allButFirst ].
    extension := ((aFilename copyAfterLast: $.)
        select: [ :each | each isAlphaNumeric ]) asLowercase capitalized.
    ^ (mainPart, extension) asSymbol

Original comment by wemb...@instantiations.com on 14 Apr 2014 at 9:06

  • Added labels: ****
  • Removed labels: ****

@GoogleCodeExporter
Copy link
Author

Original comment by philippe...@gmail.com on 21 Apr 2014 at 9:14

  • Changed state: Started
  • Added labels: ****
  • Removed labels: ****

@GoogleCodeExporter
Copy link
Author

Name: Seaside-Core-pmm.814
Author: pmm
Time: 21 April 2014, 11:29:09 am
UUID: ea3b7fa1-33b0-4183-b4b0-3e5f12c78ae6
Ancestors: Seaside-Core-JohanBrichau.813

Issue 786:  Bug in WAAbstractFileLibrary
- better handle "strange" file names that can be provided through the web

Name: Seaside-Tests-Core-pmm.275
Author: pmm
Time: 21 April 2014, 11:29:42 am
UUID: 38e62f27-f000-4fc6-97b0-e2bc3d4f1a0c
Ancestors: Seaside-Tests-Core-JohanBrichau.274

Issue 786:  Bug in WAAbstractFileLibrary
- better handle "strange" file names that can be provided through the web

Original comment by philippe...@gmail.com on 21 Apr 2014 at 9:29

  • Changed state: Fixed
  • Added labels: ****
  • Removed labels: ****

@GoogleCodeExporter
Copy link
Author

Hi,

our server just got another request that broke even this implementation: 
/files/JQUiDeploymentLibrary/%29%2C10%29;n=isNaN%28n%29?0%3An%3Bp=this._get%28b%
2C
The reason is that mainPart ends up being a String of only Digits, so the 
[mainPart first isDigit] still breaks.

I don't know what your fix looks like, but if you used the suggestion from John 
and myself, it's still insufficient.

The method should be changed to

asSelector: aFilename
    | mainPart extension |
    mainPart := (aFilename copyUpToLast: $.)
        select: [ :each | each isAlphaNumeric ].

    mainPart isEmptyOrNil ifTrue: [ ^ nil ].

    [mainPart notEmpty and: [mainPart first isDigit]]
        whileTrue: [ mainPart := mainPart allButFirst ].

    mainPart isEmptyOrNil ifTrue: [ ^ nil ].

    extension := ((aFilename copyAfterLast: $.)
        select: [ :each | each isAlphaNumeric ]) asLowercase capitalized.
    ^ (mainPart, extension) asSymbol 

Joachim

Original comment by jtuc...@objektfabrik.de on 19 May 2014 at 12:25

  • Added labels: ****
  • Removed labels: ****

@GoogleCodeExporter
Copy link
Author

Good catch

Name: Seaside-Core-pmm.815
Author: pmm
Time: 19 May 2014, 9:12:19 pm
UUID: ac051160-4c44-4a89-9bdf-b0bdb25dd23a
Ancestors: Seaside-Core-pmm.814

Issue 786:  Bug in WAAbstractFileLibrary
- https://code.google.com/p/seaside/issues/detail?id=786
- fix for fix

Name: Seaside-Tests-Core-pmm.278
Author: pmm
Time: 19 May 2014, 9:12:50 pm
UUID: 3b92a899-db09-4f03-9d9e-cebab8693684
Ancestors: Seaside-Tests-Core-JohanBrichau.277

Issue 786:  Bug in WAAbstractFileLibrary
- https://code.google.com/p/seaside/issues/detail?id=786
- fix for fix

Original comment by philippe...@gmail.com on 19 May 2014 at 7:12

  • Added labels: ****
  • Removed labels: ****

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant