Rewrite recorded Location only if it is a URI #461
Conversation
Codecov Report
@@ Coverage Diff @@
## master #461 +/- ##
==========================================
+ Coverage 23.25% 23.39% +0.13%
==========================================
Files 6 6
Lines 1135 1137 +2
Branches 173 173
==========================================
+ Hits 264 266 +2
Misses 854 854
Partials 17 17
Continue to review full report at Codecov.
|
| # Bad assumption that the URI-M will contain \d14 but works for now. | ||
| uriBeforeURIR = request.url[:re.search(r'/\d{14}/', request.url).end()] | ||
| newURIM = uriBeforeURIR + resp.headers['Location'] | ||
| resp.headers['location'] = newURIM | ||
| resp.headers['Location'] = newURIM |
There was a problem hiding this comment.
I experimented with this prior to using all-lower case and there might be some API-level normalization on retrieving this header name.
There was a problem hiding this comment.
I have tested it locally exhaustively. The dictionary-like implementation here is case-insensitive. I have changed the case in this line just to be consistent.
There was a problem hiding this comment.
It's good to have confirmation of this, @ibnesayeed. I experienced the same behavior with the dictionary-like implementation provided by Flask. I have no issue with this change to Location.
| @@ -691,15 +691,19 @@ def handler(signum, frame): | |||
| # respWithLinkHeader = getLinkHeaderAbbreviatedTimeMap(path, datetime) | |||
| # resp.headers['Link'] = respWithLinkHeader.replace('\n', ' ') | |||
|
|
|||
| if status[0] == '3': | |||
| if status[0] == '3' and isUri(resp.headers.get('Location')): | |||
There was a problem hiding this comment.
Location: /framentURI will fail the isURI check. Where is this condition handled?
There was a problem hiding this comment.
That's the point of this check. We do not want to rewrite Location if it is not a full URI. SW should be able to handle that condition.
There was a problem hiding this comment.
SW should be able to handle that condition
Does Reconstruct handle this condition with this line excluded? I have yet to test it. Have you?
There was a problem hiding this comment.
No, I have not tested it, but it should work. Even if it doesn't, the functionality is better than before where were incorrectly rewriting.
There was a problem hiding this comment.
By the way, I used resp.headers.get('Location') instead of resp.headers['Location'] in the if condition to avoid any exceptions if for whatever reason the Location header was not present in the captures record although it had 3xx status.
|
@ibnesayeed Will this fix #456? Particularly records that have an HTTP response header of |
That's the expectation, if the SW part works the way I think it would. I have not tested it by replaying any such pages. I would encourage you to test that. |
|
@machawk1 did you get a chance to test this one? |
|
@ibnesayeed I still do not experience replay of redirects to URI fragments correctly. Visit memento at ...but on replaying the sole memento, I am forwarded to ipwb commit f909b4c |
Fix #456, related #452
Changes are untested.