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

Set cache-control headers to reduce server load (fixes #412) #1641

Merged
merged 5 commits into from Jun 28, 2023

Conversation

Nutomic
Copy link
Member

@Nutomic Nutomic commented Jun 27, 2023

See code comment for explanation.

@SleeplessOne1917
Copy link
Member

I don't see the code comment you're referring to in the changes.

@Nutomic
Copy link
Member Author

Nutomic commented Jun 27, 2023

Sorry I forgot to commit some changes, check again now.

Copy link
Member

@SleeplessOne1917 SleeplessOne1917 left a comment

Choose a reason for hiding this comment

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

There's a small change to make, but it's not consequential enough to mark as request changes.

src/server/middleware.ts Outdated Show resolved Hide resolved
@alectrocute
Copy link
Contributor

I’m not a fan of all the middleware being in the same file. We should keep it a folder, yes?

const user = UserService.Instance;
let caching;
if (user.auth()) {
caching = "private";
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think a max-age could be useful here as well

Copy link
Member Author

Choose a reason for hiding this comment

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

That will cause problems if you write a post, then go to the post listing which is cached, and is missing your new post. In any case the main server load is probably from unauthenticated users so this should already help a lot.

@alectrocute
Copy link
Contributor

Does this resolve #1375?

@alectrocute alectrocute added this to the 0.18.1 milestone Jun 27, 2023
}) {
res.setHeader(
"Content-Security-Policy",
`default-src 'self'; manifest-src *; connect-src *; img-src * data:; script-src 'self' 'unsafe-inline' 'unsafe-eval'; style-src 'self' 'unsafe-inline'; form-action 'self'; base-uri 'self'; frame-src *; media-src *`

Choose a reason for hiding this comment

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

Are these really necessary?

unsafe-inline and unsafe-eval introduce serious security risks.

Please avoid usage or use with nonce

Copy link
Member Author

Choose a reason for hiding this comment

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

Afaik they are required by the js framework, would be great if we could get rid of them.

Copy link
Member

Choose a reason for hiding this comment

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

@pixlguru you can try removing these as a separate PR, but make sure you do real testing on it.

Copy link

@pixlguru pixlguru Jun 28, 2023

Choose a reason for hiding this comment

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

Maybe I could do this after 18.1 is out, if I remember correctly ,last time I took a look unsave-eval was used by websocket code, so there is a chance that removing it has no side-effects.

@dessalines dessalines merged commit 32063a5 into main Jun 28, 2023
2 checks passed
@pixlguru pixlguru mentioned this pull request Jul 10, 2023
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

6 participants