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

Add example of XSS vulnerability #24

Merged
merged 26 commits into from
Oct 26, 2022
Merged

Add example of XSS vulnerability #24

merged 26 commits into from
Oct 26, 2022

Conversation

dschwarz91
Copy link
Contributor

hey karl,
just added an xss example.
For now, i just put the payload in an additional row in the blog_posts table.
Should we put that in an own table or should we add an additional column to the blog_posts table to somehow mark this entry? What do you prefer?

@karlhorky
Copy link
Member

@dschwarz91 cool, this is a great idea! What do you think about also including:

  1. A demo of XSS in a React HTML prop? (explanation, CodeSandbox, official docs showing deprecation)
  2. Solution 3: An additional database row using Markdown instead of HTML?
  3. Does it make sense to show / mention TrustedHTML already? Or too new still?

@dschwarz91
Copy link
Contributor Author

@karlhorky: thx for the feedback!

1. A demo of XSS in a React HTML prop? 

I've already had an additional example with the deprecated javascrip: URLs, but since there are no really nice fixes for it (at least I dont know about any - besides "stealing" the filter-regex from angular ^^), I decided to just include it in the slides, but not in the example repo.

Wasn't aware of the way via "props: " tho - will have a look at that.

2. Solution 3: An additional database row using Markdown instead of HTML?

good Idea, will add that =)

3. Does it make sense to show / mention [TrustedHTML] already? Or too new still?

It absolutely makes sense and I actually already tried to include it in the examples but failed ^^
I wanted to enable it only on a specific sub-page (solution3) via (the next.js feature)

and it kinda worked (because I could see some trusted types errors in the console), but somehow react seemed to render the page (and also the payload) once before it activated trusted types.
So it seemed as it would be necessary to enable it for the whole application.
And then I decided to just mention it in the slides. (still no skilled react dev ^^)
But if you have any idea how to just enable it on a single react page, I'm happy to try again =)

@karlhorky
Copy link
Member

karlhorky commented Oct 20, 2022

  1. A demo of XSS in a React HTML prop?

I've already had an additional example with the deprecated javascrip: URLs, but since there are no really nice fixes for it (at least I dont know about any - besides "stealing" the filter-regex from angular ^^), I decided to just include it in the slides, but not in the example repo.

Maybe some kind of form that submits content to the database which also includes a new url field in the database?

But yeah, totally understand if this is outside the scope of this PR - if you think it's too big for now, maybe you can open a new issue for it, with a clear documentation of what needs to be done with this? You may also want to link it to this issue:


  1. Solution 3: An additional database row using Markdown instead of HTML?

good Idea, will add that =)

Ok I'll wait on this before reviewing again then.


  1. Does it make sense to show / mention [TrustedHTML] already? Or too new still?

It absolutely makes sense and I actually already tried to include it in the examples but failed
...
And then I decided to just mention it in the slides.

Fine with me, no problem there! Maybe you can open an issue for this too?

@dschwarz91
Copy link
Contributor Author

ok, added the markdown example and created an issue for an URL example (#28) and a Trusted Types example (#27)

util/cookies.ts Outdated Show resolved Hide resolved
pages/_app.tsx Outdated Show resolved Hide resolved
@karlhorky karlhorky changed the title added an XSS example Add example of XSS vulnerability Oct 24, 2022
package.json Outdated Show resolved Hide resolved
database/blogPosts.ts Outdated Show resolved Hide resolved
database/blogPosts.ts Outdated Show resolved Hide resolved
karlhorky
karlhorky previously approved these changes Oct 24, 2022
@karlhorky karlhorky dismissed their stale review October 24, 2022 16:29

not ready yet

@karlhorky karlhorky merged commit 3805c0c into upleveled:main Oct 26, 2022
@karlhorky
Copy link
Member

Thanks for this 🙌 I addressed the points above - now merging!

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