FIX RefererResult bug #975
Conversation
the url host an invalid referer was returned)
the referer host an invalid referer was returned
…present in" This reverts commit f41182d.
…resent in" This reverts commit fa7065c.
If the context path string was present in the referer host, the getReferer() method returned an invalid referer
On proxied app servers is common to have the context path suppressed from the external URL, although it is still valid within the app server context. If the request referer path does not start with the internal context, it should return the entire path.
Fix referer retrieval bug
|
|
||
| verify(logic).forwardTo(RefererController.class); | ||
| verify(controller).index(); | ||
| } |
There was a problem hiding this comment.
Too much context to execute a simple check. Maybe it's better to relax visibility of getReferer() to protected and test only this method, what do you think?
and we get an extra extension point.
There was a problem hiding this comment.
Well, that was my first commit on VRaptor so I tried to write comprehensive unit tests :-) - but I agree it's too much for something so simple. In addition it's a hard condition to achieve, the strings must match in a little-prob coincidence.
About the extension point, I don't have an opinion - would you expect this class to be extended? If so, then yes it's a good way to maintain its structure (or else people would have to create another method to perform this check, or even put its code within the callers)
There was a problem hiding this comment.
Your tests are fine =)
it's just that this test class has several tests that could be testing only getReferer(), not the whole redirect or forward methods.
There was a problem hiding this comment.
You're right, I followed the other methods pattern but we could have simple tests which would check if the returned string matches the expected value and we're good to go. Wanna me to change that?
There was a problem hiding this comment.
Yes, please @valeriolopes! After that change we can merge this PR.
There was a problem hiding this comment.
I ended up creating another pull request (976); I really need to stop and play with git (we still use svn here). The changes are there.
There was a problem hiding this comment.
Nice, thanks @valeriolopes. So I think we close this PR, right?
There was a problem hiding this comment.
Yeah, looks like the other one embraces everything
|
You're right, I followed the other methods pattern but we could have simple tests which would check if the returned string matches the expected value and we're good to go. Wanna me to change that? |
When retrieving the http referer, vraptor uses the substring method to identify the beggining of the context path. If the context path appears in the URL host the return is invalid.
Example: http://application.code.com/app/test/
VRaptor will search '/app' in the URL and will find http://application.code.com/app.test and return /application.code.com/app/test and not /app/test as expected.