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

Handle url specials from cell data in addLinksToTables #32

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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