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.

Sign up for GitHub

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

Jump to bottom

Removing URL query param from swagger UI to fix the XSS issue #23234

Merged
merged 1 commit into from
Mar 22, 2016
Merged

Removing URL query param from swagger UI to fix the XSS issue #23234

merged 1 commit into from
Mar 22, 2016

Conversation

nikhiljindal
Copy link
Contributor

Removing the URL query param from swagger UI to fix the reported XSS issue: swagger-api/swagger-ui#830 (comment).

Note that we are now disabling swagger UI by default, so this is not as critical. But it will still be good to fix the XSS for those who want to enable swagger UI on their master.

Note that there might be more XSS issues (indeed swagger-api/swagger-ui#830 had reported a bunch of them), but there was a clear repro for only this one.

@bgrant0607 @kubernetes/docs

@k8s-github-robot
Copy link

Labelling this PR as size/S

@k8s-github-robot k8s-github-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 19, 2016
@nikhiljindal
Copy link
Contributor Author

Verified that http://localhost:8080/swagger-ui/?url=http://api.ma.la/tmp/cors/swi/ works fine after this fix. It resulted in an XSS before.

@nikhiljindal
Copy link
Contributor Author

I can send the same fix to kubernetes.io if this looks good

@k8s-bot
Copy link

k8s-bot commented Mar 19, 2016

GCE e2e build/test passed for commit a9f80c2c3108e9b31f835fbde19fa2489f8ed588.

@bgrant0607
Copy link
Member

Has this been fixed upstream?

@nikhiljindal
Copy link
Contributor Author

Am not sure if upstream requires the URL query param: swagger-api/swagger-ui#830 (comment).
We do not require it for sure, so we can remove it.

Will push the change to upstream, if they are fine with removing the query param.

@nikhiljindal
Copy link
Contributor Author

They replied that they need the query param on upstream. So this is just a hack on our side

@k8s-bot
Copy link

k8s-bot commented Mar 21, 2016

GCE e2e build/test passed for commit ded122df95f8571a444ddf01f05f5a4bc2c7e854.

@bgrant0607 bgrant0607 added this to the v1.2 milestone Mar 21, 2016
@bgrant0607
Copy link
Member

LGTM. Please squash.

cc @liggitt

@nikhiljindal nikhiljindal force-pushed the swaggerRemoveurlQuery branch from ded122d to 532398a Compare March 22, 2016 17:35
@nikhiljindal
Copy link
Contributor Author

Thanks, squashed. Adding LGTM label as per comment above.

@nikhiljindal nikhiljindal added lgtm "Looks good to me", indicates that a PR is ready to be merged. e2e-not-required labels Mar 22, 2016
@k8s-bot
Copy link

k8s-bot commented Mar 22, 2016

GCE e2e build/test passed for commit 532398a.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

k8s-github-robot pushed a commit that referenced this pull request Mar 22, 2016
Merge pull request #23234 from nikhiljindal/swaggerRemoveurlQuery
b50e89e 
Auto commit by PR queue bot
@k8s-github-robot k8s-github-robot merged commit b50e89e into kubernetes:master Mar 22, 2016
@bgrant0607 bgrant0607 added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Mar 23, 2016
eparis pushed a commit to eparis/kubernetes that referenced this pull request Mar 24, 2016
@eparis
Merge pull request kubernetes#23234 from nikhiljindal/swaggerRemoveur…
5986414 
…lQuery

Auto commit by PR queue bot
(cherry picked from commit b50e89e)
@eparis eparis mentioned this pull request Mar 24, 2016
Merged
@bgrant0607 bgrant0607 added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Mar 24, 2016
@k8s-cherrypick-bot
Copy link

Commit 5986414 found in the "release-1.2" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this s an error find help to get your PR picked.

AlainRoy pushed a commit to vmware-archive/kubernetes-archived that referenced this pull request Mar 29, 2016
Merge pull request kubernetes#23234 from nikhiljindal/swaggerRemoveur…
814bfc1 
…lQuery

Auto commit by PR queue bot
(cherry picked from commit b50e89e)
alena1108 pushed a commit to rancher/kubernetes that referenced this pull request May 20, 2016
Merge pull request kubernetes#23234 from nikhiljindal/swaggerRemoveur…
6b8ac33 
…lQuery

Auto commit by PR queue bot
(cherry picked from commit b50e89e)
shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016
@eparis
Merge pull request kubernetes#23234 from nikhiljindal/swaggerRemoveur…
1b37979 
…lQuery

Auto commit by PR queue bot
(cherry picked from commit b50e89e)
shouhong pushed a commit to shouhong/kubernetes that referenced this pull request Feb 14, 2017
@eparis
Merge pull request kubernetes#23234 from nikhiljindal/swaggerRemoveur…
67c9550 
…lQuery

Auto commit by PR queue bot
(cherry picked from commit b50e89e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@bgrant0607 bgrant0607

Labels
cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v1.2
Development

Successfully merging this pull request may close these issues.
7 participants
@nikhiljindal @k8s-github-robot @k8s-bot @bgrant0607 @k8s-cherrypick-bot @k8s-teamcity-mesosphere @googlebot