Skip to content

Conversation

@pwildani
Copy link

This probably addresses #31, but I encountered it independently.

When the cell contents includes URL-special characters, they should probably
not be interpreted as multiple query parameters or other such errors.

For example, with a addLinksToTables({link_columns: ["col"], link_urls: ["http://modeanalytics?param_foo={{col}}"]}):

  • Cell Contents: A+B&securityhole=true
  • Before: http://modeanalytics/?param_foo=A+B&securityhole=true
  • After: http://modeanalytics/?param_foo=A%2BB%26securityhole=true

So this avoids data that is potentially sourced from external users getting
injected into the super dangerous "securityhole" parameter on another report
unintentionally.

Or more practically, when the data contains a +, it doesn't get re-interpreted
as a space by the browser.

IMO, interpreting data from a single cell in a table as multiple parameters
should be a special case that needs more coding. The simple case of expecting
the cell contents to be a single scalar value should be the default.

When the cell contents includes URL-special characters, they should probably
not be interpreted as multiple query parameters or other such errors.

For example, with a addLinksToTables({link_columns: ["col"], link_urls: ["http://modeanalytics?param_foo={{col}}"]}):
  Cell Contents: `A+B&securityhole=true`
  Before: `http://modeanalytics/?param_foo=A+B&securityhole=true`
  After: `http://modeanalytics/?param_foo=A%2bB%26securityhole=true`

So this avoids data that is potentially sourced from external users getting
injected into the super dangerous "securityhole" parameter on another report
unintentionally.

Or more practically, when the data contains a +, it doesn't get re-interpreted
as a space by the browser.

IMO, interpreting data from a single cell in a table as multiple parameters
should be a special case that needs more coding. The simple case of expecting
the cell contents to be a single scalar value should be the default.
@leqilong
Copy link
Contributor

Hi @pwildani! Thanks for the PR. I think this code change assumes that the cell content will be the value of a query string. However, there are cases where the cell content are a URLs, like this report: https://modeanalytics.com/modeanalytics/reports/0bc75fadb03e/runs/d94183dc4088 In this case, the line encodeURIComponent(content) would encode the whole URL, which would make the link broken.

@pwildani
Copy link
Author

Tricky. Handling that difference might need a real behavior controlling flag or a separate top level function.

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