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

Use the correct protocol for S3 attachment URLs based on the request security level #528

Merged
merged 2 commits into from
Jan 18, 2014

Conversation

hprange
Copy link
Contributor

@hprange hprange commented Jan 16, 2014

Problem: the generated URL for S3 attachments always uses the HTTP protocol. In the context of secure requests (made over HTTPS), the generated URL should use the HTTPS protocol instead.

Solution: Chosing the URL protocol during persistence of the attachment is not ideal because the attachment may be used in secured and unsecured contexts later. The correct protocol should be determined during the URL generation. This patch replaces the HTTP protocol and removes the port (:80) from the generated URL whenever the request is secure.

…security level

Problem: the generated URL for S3 attachments always uses the HTTP protocol. In the context of secure requests (made over HTTPS), the generated URL should use the HTTPS protocol instead.

Solution: Chosing the URL protocol during persistence of the attachment is not ideal because the attachment may be used in secured and unsecured contexts later. The correct protocol should be determined during the URL generation. This patch replaces the HTTP protocol and removes the port (:80) from the generated URL whenever the request is secure.
@darkv
Copy link
Member

darkv commented Jan 16, 2014

Hi Henrique,

instead of fixing the String wouldn't it make sense to get the correct URL in the first place? What about replacing the line 142 with:

attachmentUrl = context.completeURLWithRequestHandlerKey(ERAttachmentRequestHandler.REQUEST_HANDLER_KEY, attachmentUrl, null, request.isSecure(), 0);

That should respect the secure setting. Does that work?

@hprange
Copy link
Contributor Author

hprange commented Jan 17, 2014

Hi Johann,

Thanks for your feedback.

The original code with the urlWithRequestHandlerKey method already decides if the request is secure or not internally. I haven't verified it, but I believe the behavior is correct when the attachment is proxied. When the attachment is not proxied, however, the URL is returned from the s3Path property without any special treatment. Something like this http://s3.amazonaws.com:80/....

Now I see I wasn't clear about the fact that this patch fixes the problem when the attachment is not proxied. In this case, replacing the s3Path contents looks like a simple solution to the problem.

What do you think?

@darkv
Copy link
Member

darkv commented Jan 17, 2014

If I got it right you need to fix the URL if the delivery is done directly by S3 instead of WO proxying the request. In that case if you access WO via HTTPS the URL for S3 should be HTTPS too but isn't because of the HTTP constant in S3_URL, correct?

So it would be better to put the if clause in line 155 in an else as this would correspond to !attachment.proxied() (see line 134).

@hprange
Copy link
Contributor Author

hprange commented Jan 17, 2014

Done.

darkv added a commit that referenced this pull request Jan 18, 2014
Use the correct protocol for S3 attachment URLs based on the request security level
@darkv darkv merged commit e58d406 into wocommunity:master Jan 18, 2014
@hprange hprange deleted the bugfix/erattachment-s3-secure-url branch January 20, 2014 00:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants