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

Escape request params. #360

Merged
merged 1 commit into from
Feb 23, 2022
Merged

Escape request params. #360

merged 1 commit into from
Feb 23, 2022

Conversation

ADmad
Copy link
Member

@ADmad ADmad commented Feb 23, 2022

Mitigate potential XSS issue.

@ADmad ADmad requested a review from ndm2 February 23, 2022 07:35
@ADmad ADmad force-pushed the ADmad-patch-1 branch 2 times, most recently from 1bd707f to abef679 Compare February 23, 2022 07:37
@ADmad ADmad changed the title Sanitize request params. Escape request params. Feb 23, 2022
@ndm2
Copy link
Collaborator

ndm2 commented Feb 23, 2022

Although I'm not sure how it would be possible for it to slip in there, Salines said the problem also occurred on other pages like login. Just in case, the example layouts should maybe apply the same escaping to the request params.

@codecov
Copy link

codecov bot commented Feb 23, 2022

Codecov Report

Merging #360 (6fe6ddb) into master (b622d37) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #360   +/-   ##
=========================================
  Coverage     99.19%   99.19%           
  Complexity      335      335           
=========================================
  Files            20       20           
  Lines           869      869           
=========================================
  Hits            862      862           
  Misses            7        7           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b622d37...6fe6ddb. Read the comment docs.

@ADmad
Copy link
Member Author

ADmad commented Feb 23, 2022

I too don't see how the problem could occur for other pages besides error pages but still escaped them for all layouts.

@ndm2
Copy link
Collaborator

ndm2 commented Feb 23, 2022

Yeah, better safe than sorry I guess. I've tried to reproduce it with non-error pages, but no luck 🤷‍♂️

There's one left hiding in the dashboard's header

<h1 class="page-header"><?= $this->request->getParam('controller'); ?></h1>

Mitigate potential XSS issue.
@ADmad ADmad merged commit 8fca5c2 into master Feb 23, 2022
@ADmad ADmad deleted the ADmad-patch-1 branch February 23, 2022 13:02
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.

None yet

2 participants