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

fix(iframe-common.js): properly encode referrerPageUrl param #1152

Merged
merged 1 commit into from
Oct 6, 2023

Conversation

JRonkin
Copy link
Contributor

@JRonkin JRonkin commented Sep 29, 2023

Currently, referrerPageUrl is added as a query param value without being escaped. That can cause problems on sites with tighter security restrictions. We faced this problem on several sites such as this one: https://yexttest.atlassian.net/browse/PC-208916

It can also cause functional problems when the referrer URL has query parameters. For example, a URL of http://example.com?a=1&b=2 will lead to https://answers.example.com?query=TEST&referrerPageUrl=http://example.com?a=1&b=2

In that example, &b=2 will be treated as a query parameter of the results page URL instead of being part of the referrerPageUrl value.

Currently, referrerPageUrl is added as a query param value without being escaped. That can cause problems on sites with tighter security restrictions. We faced this problem on several sites such as this one: https://yexttest.atlassian.net/browse/PC-208916

It can also cause functional problems when the referrer URL has query parameters. For example, a URL of http://example.com?a=1&b=2 will lead to https://answers.example.com?query=TEST&referrerPageUrl=http://example.com?a=1&b=2

In that example, &b=2 will be treated as a query parameter of the  results page URL instead of being part of the referrerPageUrl value.
@coveralls
Copy link

Coverage Status

coverage: 8.737%. remained the same when pulling cdbcb4e on jronkin/referrrerpageurl-encoding-fix into 6a472ed on master.

@cea2aj
Copy link
Member

cea2aj commented Oct 5, 2023

Can you change the base of this PR to the hotfix/v1.33.2 branch? Otherwise LGTM!

@JRonkin JRonkin changed the base branch from master to hotfix/v1.33.2 October 5, 2023 14:20
@JRonkin
Copy link
Contributor Author

JRonkin commented Oct 5, 2023

Can you change the base of this PR to the hotfix/v1.33.2 branch? Otherwise LGTM!

Done. There are still a couple automated tests failing. Is that expected?

@JRonkin JRonkin requested a review from cea2aj October 5, 2023 14:22
@cea2aj
Copy link
Member

cea2aj commented Oct 5, 2023

Can you change the base of this PR to the hotfix/v1.33.2 branch? Otherwise LGTM!

Done. There are still a couple automated tests failing. Is that expected?

We can ignore all of the browserstack and percy tests because we don't use those services anymore. The headless acceptance tests are still used, however those test seem to behave very unpredictably based on the last few commits in master. I re-ran the headless acceptance tests to see if any more pass

@JRonkin
Copy link
Contributor Author

JRonkin commented Oct 5, 2023

We can ignore all of the browserstack and percy tests because we don't use those services anymore. The headless acceptance tests are still used, however those test seem to behave very unpredictably based on the last few commits in master. I re-ran the headless acceptance tests to see if any more pass

Still failed. Should we ignore that too?

@cea2aj
Copy link
Member

cea2aj commented Oct 5, 2023

Still failed. Should we ignore that too?

Only one test failed this time instead of eight, which is the same as the master branch which suggests that the issue isn't due to this change. So we can ignore that failing test

@JRonkin JRonkin merged commit 2f8fe5e into hotfix/v1.33.2 Oct 6, 2023
16 of 27 checks passed
@JRonkin JRonkin deleted the jronkin/referrrerpageurl-encoding-fix branch October 6, 2023 20:05
@nmanu1 nmanu1 mentioned this pull request Dec 8, 2023
nmanu1 added a commit that referenced this pull request Dec 8, 2023
### Fixes
- Added encoding for the `referrerPageUrl` param so it doesn't affect the main URL's query params and works better with some tighter security restrictions (#1152)
- Upgraded `@babel/traverse` version to address a vulnerability in the package (#1156)
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.

3 participants