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

feat: Improve Application Security #1889

Merged
merged 20 commits into from
Jan 14, 2025
Merged

feat: Improve Application Security #1889

merged 20 commits into from
Jan 14, 2025

Conversation

adintegra
Copy link
Contributor

@adintegra adintegra commented Nov 18, 2024

Tightening up security of the application in general:

  • Implement CSP
  • Better sanitisation of GraphQL queries to ensure only permitted datasources are used (addresses BAFU vulnerability report)

Copy link

vercel bot commented Nov 18, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
visualization-tool ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 14, 2025 10:55am

@adintegra adintegra changed the title feat: Adds basic CSP configuration feat: Adds Content Security Policy Nov 18, 2024
@adintegra adintegra linked an issue Nov 19, 2024 that may be closed by this pull request
@adintegra
Copy link
Contributor Author

@wereHamster, making these changes primarily due to the BAFU's report, but also generally, to improve overall security. Do you think the added headers make sense?

@adintegra adintegra changed the title feat: Adds Content Security Policy feat: Improve Application Security Dec 5, 2024
@ptbrowne
Copy link
Collaborator

ptbrowne commented Dec 7, 2024

I added validation of the data source url so that we could only used urls that are whitelisted in datasourceUrl.

I tried with https://www.npmjs.com/package/graphql-constraint-directive but could not make it work and it seems the directive only worked if the field was nested inside an object, which would have required a refactor of places using datasourceUrl.

Instead, I used a custom scalar, which also can be used for providing additional validation rules on existing scalars (see https://www.apollographql.com/docs/apollo-server/schema/custom-scalars).

@adintegra
Copy link
Contributor Author

Thanks @ptbrowne! Looks like this would be a good solution 👍🏼

Did you have issues with graphql-constraint-directive because of the version maybe? I initially missed the note about having to use v2 with graphql v15.

@ptbrowne
Copy link
Collaborator

ptbrowne commented Dec 7, 2024

I do not know, I did not see this note either, could be. Did it work for you ?

# Conflicts:
#	app/graphql/query-hooks.ts
#	app/graphql/resolver-types.ts
#	yarn.lock
Copy link
Collaborator

@bprusinowski bprusinowski left a comment

Choose a reason for hiding this comment

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

Thanks @adintegra, I am not 100% sure how to test the change, but overall looks good!

One maintenance thing I'd consider would be to add a CHANGELOG entry, mentioning that the security of the application has been improved 🔒

app/domain/datasource/urls.ts Outdated Show resolved Hide resolved
app/graphql/resolvers/index.ts Show resolved Hide resolved
app/next.config.js Show resolved Hide resolved
@adintegra adintegra merged commit ae94562 into main Jan 14, 2025
5 of 7 checks passed
@adintegra adintegra deleted the feat/implement-csp branch January 14, 2025 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vulnerability Report (SLA)
3 participants