Skip to content
Permalink
Browse files Browse the repository at this point in the history
Fix #46244 Remove innerHTML usage to avoid self-XSS
  • Loading branch information
codergeek121 committed Oct 21, 2022
1 parent be0b5c6 commit be177e4
Showing 1 changed file with 17 additions and 8 deletions.
Expand Up @@ -102,9 +102,9 @@
// Enables path search functionality
function setupMatchPaths() {
// Check if there are any matched results in a section
function checkNoMatch(section, noMatchText) {
function checkNoMatch(section, trElement) {
if (section.children.length <= 1) {
section.innerHTML += noMatchText;
section.appendChild(trElement);
}
}

Expand Down Expand Up @@ -145,21 +145,30 @@
}
}

function buildTr(string) {
var tr = document.createElement('tr');
var th = document.createElement('th');
th.setAttribute('colspan', 4);
tr.appendChild(th);
th.innerText = string;
return tr;
}

// On key press perform a search for matching paths
delayedKeyup(searchElem, function() {
var path = sanitizePath(searchElem.value),
defaultExactMatch = '<tr><th colspan="4">Paths Matching (' + path +'):</th></tr>',
defaultFuzzyMatch = '<tr><th colspan="4">Paths Containing (' + path +'):</th></tr>',
noExactMatch = '<tr><th colspan="4">No Exact Matches Found</th></tr>',
noFuzzyMatch = '<tr><th colspan="4">No Fuzzy Matches Found</th></tr>';
defaultExactMatch = buildTr('Paths Matching (' + path + '):'),
defaultFuzzyMatch = buildTr('Paths Containing (' + path +'):'),
noExactMatch = buildTr('No Exact Matches Found'),
noFuzzyMatch = buildTr('No Fuzzy Matches Found');

if (!path)
return searchElem.onblur();

getJSON('/rails/info/routes?path=' + path, function(matches){
// Clear out results section
exactSection.innerHTML = defaultExactMatch;
fuzzySection.innerHTML = defaultFuzzyMatch;
exactSection.replaceChildren(defaultExactMatch);
fuzzySection.replaceChildren(defaultFuzzyMatch);

// Display exact matches and fuzzy matches
pathElements.forEach(function(elem) {
Expand Down

14 comments on commit be177e4

@ejdiener
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure github, whatever you say

@dgm
Copy link
Contributor

@dgm dgm commented on be177e4 Dec 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course, this page should never be triggered in a production environment anyway.

@simi
Copy link
Contributor

@simi simi commented on be177e4 Dec 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's wrong with making "dev" pages safe?

@dgm
Copy link
Contributor

@dgm dgm commented on be177e4 Dec 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can do what you want, but I'm not losing any sleep over this.

@kwakuboateng-dev
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What am i supposed to do about this? lol

@efrapp
Copy link

@efrapp efrapp commented on be177e4 Jan 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does someone know if the Rails team is planning a patch for Rails 6 in response to this vulnerability GHSA-9chr-4fjh-5rgw?
I know the commit was merged into the main branch, but I don't think the patch applies retroactively for older versions.
Thanks in advance for your answer.


Additionally, I saw this PR d35b1a8 targeting Rails 5.2 but it was closed.

@rafaelfranca
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no CVE here. Who created it? it is a development page, even if there was a XSS here, the impact is so low that there is no reason to call this a vulnerability.

@efrapp
Copy link

@efrapp efrapp commented on be177e4 Jan 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a notification we get from Github with periodic CVEs in different gems we use in our projects. Not sure who created it but it is listed on the National Vulnerability Database page: https://nvd.nist.gov/vuln/detail/CVE-2022-3704.

@rafaelfranca
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I saw that. We are disputing that CVE since it is invalid.

@efrapp
Copy link

@efrapp efrapp commented on be177e4 Jan 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Ok, let's get rid of the CVE thing, and let me put my question from another perspective. Is there a way to get this commit patch (PR associated: #46269) into the Rails 6 version?

@skipkayhil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It already is here: 5a7fa9a

@efrapp
Copy link

@efrapp efrapp commented on be177e4 Jan 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @skipkayhil, I had to be more precise. The version we are using is 6.1.7 and I saw that the rails 6-1-stable branch already has this patch too, but when I check the tag v1.7.6 the change is not there: https://github.com/rails/rails/blob/v6.1.7/actionpack/lib/action_dispatch/middleware/templates/routes/_table.html.erb#L116.
I don't know the internal process the Rails team uses to apply the patches so what do you suggest to me to get this fix in the version we are using?

@skipkayhil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gem "rails", github: "rails/rails", branch: "6-1-stable"

@efrapp
Copy link

@efrapp efrapp commented on be177e4 Jan 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thank you so much. I'll discuss this solution with the team.

Please sign in to comment.