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

Docs: add security section #416

Merged
merged 2 commits into from
Jul 25, 2023
Merged

Docs: add security section #416

merged 2 commits into from
Jul 25, 2023

Conversation

AgnesToulet
Copy link
Contributor

This is a start, I don't know if this is what you had in mind when you wrote #388

Closes #388

Copy link
Collaborator

@joanlopez joanlopez left a comment

Choose a reason for hiding this comment

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

It looks like a great start, few things I'm wondering if would make sense to add are:

  • Give few more details about the reasons why to set this up, like "otherwise something like this could happen", and refer the security incident.
  • Perhaps mentioning that it's configured to - by default, so it's:
    • Not safe, because any potential attacker would start with - as secret token.
    • Therefore, it must be replaced.
    • But, if you get started, you'll notice that it requires a secret token to authorize rendering requests.

@joanlopez
Copy link
Collaborator

Indeed, by looking at the code that checks for the token (for instance, changes here), it seems it always checks for equality and empty string is not considered a valid (incoming) auth token. Therefore, perhaps we may need to slightly change the wording to say this is mandatory, at least with the default value -, which we strongly recommend to modify.

WDYT?

@joanlopez
Copy link
Collaborator

joanlopez commented Jul 13, 2023

Hi @AgnesToulet, is there anything else missing?
Honestly, if we don't have any clear idea to add more details, I'd merge it as-is (better to have at least something), and perhaps iterate it later, if needed.

@AgnesToulet
Copy link
Contributor Author

Sorry I missed your review on this. I updated the README according to your feedback, thanks!

@joanlopez
Copy link
Collaborator

Sorry I missed your review on this. I updated the README according to your feedback, thanks!

Don't worry at all! Looks great! :)

@joanlopez joanlopez merged commit 57870b2 into master Jul 25, 2023
4 checks passed
@joanlopez joanlopez deleted the docs/add-security branch July 25, 2023 15:13
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.

Add auth_token to install instruction
2 participants