Skip to content
This repository was archived by the owner on Oct 11, 2022. It is now read-only.

Conversation

@brianlovin
Copy link
Contributor

Status

  • WIP
  • Ready for review
  • Needs testing

Deploy after merge (delete what needn't be deployed)

  • api

@mxstbr this is a stopgap fix for #4667 - we should ship this anyways. It follows your advice of just handling the fix at the query layer. We detect if we stored an imgix url in the thread body, strip the query params, and re-sign it. I confirmed that this works locally.

A followup PR I can work on tomorrow if you approve this is to implement the rest of the steps in #4667 to make sure we don't store signed images in the db during publish or edit.

mxstbr
mxstbr previously approved these changes Feb 20, 2019
Copy link
Contributor

@mxstbr mxstbr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Workaround LGTM, just have to fix CI!

@brianlovin
Copy link
Contributor Author

Fixed - we just have to fallback to a bad src in case url.parse() fails for whatever reason

Copy link
Contributor

@mxstbr mxstbr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if we are sure that the fallback behaviour can not be abused for malicious purposes!

@brianlovin
Copy link
Contributor Author

LGTM if we are sure that the fallback behaviour can not be abused for malicious purposes!

Afaik this isn't possible, the fallback will only be sent to the client if url.parse(...).pathname returns undefined or null

@brianlovin brianlovin merged commit deb9b30 into alpha Feb 20, 2019
@brianlovin brianlovin deleted the space-in-uploads branch February 20, 2019 17:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants