Redirect /serviceworker.js to assets/ #15823

zeripath · 2021-05-10T19:15:30Z

Since we moved the assets to the /assets there are multiple requests to serviceworker.js that are getting 404'd.

This PR simply redirects requests to /assets.

Signed-off-by: Andrew Thornton art27@cantab.net

Signed-off-by: Andrew Thornton <art27@cantab.net>

silverwind · 2021-05-10T23:23:45Z

I think this may be old serviceworker instances that are trying to self-update. The new ones should be on the correct path. It may be possible to cleanly unregister the old instances from JS, but I'm not exactly sure how to do it.

silverwind · 2021-05-10T23:27:10Z

routers/routes/web.go

@@ -162,6 +162,9 @@ func WebRoutes() *web.Route {
 	// We use r.Route here over r.Use because this prevents requests that are not for avatars having to go through this additional handler
 	routes.Route("/avatars/*", "GET, HEAD", storageHandler(setting.Avatar.Storage, "avatars", storage.Avatars))
 	routes.Route("/repo-avatars/*", "GET, HEAD", storageHandler(setting.RepoAvatar.Storage, "repo-avatars", storage.RepoAvatars))
+	routes.Route("/serviceworker.js", "GET, HEAD", http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) {
+		http.Redirect(resp, req, setting.AppURL+"assets/serviceworker.js", http.StatusFound)


Suggested change

http.Redirect(resp, req, setting.AppURL+"assets/serviceworker.js", http.StatusFound)

http.Redirect(resp, req, setting.AppURL+"assets/serviceworker.js", http.StatusTemporaryRedirect)

We should only use 307 or 308 redirects because those forbid method change which is better for both functionality and security.

I'm not certain I understand how security or functionality could affected by posts etc being redirected to assets - in fact it would be better if they were.

Does the web browser support the redirect?

It does not matter for GET requests but for other methods, browsers may decide to change method to GET based on obscure criteria related to HTML forms which I think is an outdated concept. See https://developer.mozilla.org/en-US/docs/Web/HTTP/Redirections#temporary_redirections for more details.

Security can be impacted of POST to a endpoint with credentials where a change to GET would move potentially sensitive parameters from request body to request URL, adding them to the browser history and making them visible in server logs. For that reason alone, I never use 301, 302 or 303, only 307 and 308 which of course all browsers support.

a1012112796 · 2021-05-11T01:23:46Z

Don't forgot add serviceworker.js to reservedUsernames list also.

a1012112796 · 2021-05-11T01:36:14Z

I think it's okay to add routes /serviceworker.js only, but should limit scope

example:

navigator.serviceWorker.register(`${AppSubUrl}/serviceworker.js`, {scope: '/assets'}),

zeripath · 2021-05-11T05:22:28Z

Don't forgot add serviceworker.js to reservedUsernames list also.

As far as I understood it already was.

zeripath · 2021-05-11T05:23:06Z

I think it's okay to add routes /serviceworker.js only, but should limit scope

example:
navigator.serviceWorker.register(`${AppSubUrl}/serviceworker.js`, {scope: '/assets'}),

I don't understand what you mean.

zeripath · 2021-05-11T05:26:34Z

I think this may be old serviceworker instances that are trying to self-update. The new ones should be on the correct path. It may be possible to cleanly unregister the old instances from JS, but I'm not exactly sure how to do it.

Yes I'm not certain why these requests are being made either. It could be due to chrome's setting UpdateOnReload in the developer settings but I don't understand why it would actually request this. AFAIU the service worker should now be embedded.

However given this repeated querying I think we do have to redirect it.

silverwind · 2021-05-11T08:01:07Z

I think we just should instead cleanly unregister all service workers in JS that are not on the current scope. Quick test on try.gitea.io shows two active service workers since the /assets change:

(await navigator.serviceWorker.getRegistrations()).map(r => r.scope)
// => [ "https://try.gitea.io/", "https://try.gitea.io/assets/" ]

I will try to come up with something.

silverwind · 2021-05-11T08:24:39Z

I think it's okay to add routes /serviceworker.js only, but should limit scope

Adding a scope parameter is unnecessary, it will default to the directory of the loaded script, e.g. /assets:

By default, the scope value for a service worker registration is set to the directory where the service worker script is located

zeripath · 2021-05-11T17:35:10Z

agh it must have been moved out when we did the assets change - thanks @a1012112796 I'll add it back in. I don't think we really want to allow a user to be named serviceworker.js anyway.

Hopefully @silverwind will figure out how to make this PR completely unnecessary.

Signed-off-by: Andrew Thornton <art27@cantab.net>

silverwind · 2021-05-11T17:53:18Z

Alternative PR: #15834

With the addition of the /assets url, users who visited a previous version of the site now may have two active service workers, one with the old scope `/` and one with scope `/assets`. This check for serviceworkers that do not match the current script path and unregisters them. Also included is a small refactor to publicpath.js which was simplified because AssetUrlPrefix is always present now. Also it makes use of the new joinPaths helper too. Fixes: go-gitea#15823

codecov-commenter · 2021-05-11T18:09:44Z

Codecov Report

Merging #15823 (dfd5ad5) into main (d37a89e) will increase coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main   #15823      +/-   ##
==========================================
+ Coverage   43.98%   44.00%   +0.01%     
==========================================
  Files         680      681       +1     
  Lines       82212    82231      +19     
==========================================
+ Hits        36162    36182      +20     
- Misses      40134    40139       +5     
+ Partials     5916     5910       -6

Impacted Files	Coverage Δ
models/user.go	`53.15% <ø> (ø)`
routers/routes/web.go	`90.50% <0.00%> (-0.34%)`	⬇️
routers/repo/download.go	`44.26% <0.00%> (-1.51%)`	⬇️
models/repo_list.go	`77.04% <0.00%> (-0.78%)`	⬇️
services/pull/pull.go	`43.37% <0.00%> (ø)`
modules/structs/user.go	`100.00% <0.00%> (ø)`
modules/notification/mail/mail.go	`38.77% <0.00%> (ø)`
modules/setting/mime_type_map.go	`55.55% <0.00%> (ø)`
modules/setting/setting.go	`49.90% <0.00%> (+0.09%)`	⬆️
modules/convert/user.go	`94.11% <0.00%> (+0.36%)`	⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d37a89e...dfd5ad5. Read the comment docs.

* Unregister non-matching serviceworkers With the addition of the /assets url, users who visited a previous version of the site now may have two active service workers, one with the old scope `/` and one with scope `/assets`. This check for serviceworkers that do not match the current script path and unregisters them. Also included is a small refactor to publicpath.js which was simplified because AssetUrlPrefix is always present now. Also it makes use of the new joinPaths helper too. Fixes: #15823

* Unregister non-matching serviceworkers With the addition of the /assets url, users who visited a previous version of the site now may have two active service workers, one with the old scope `/` and one with scope `/assets`. This check for serviceworkers that do not match the current script path and unregisters them. Also included is a small refactor to publicpath.js which was simplified because AssetUrlPrefix is always present now. Also it makes use of the new joinPaths helper too. Fixes: go-gitea#15823

Redirect /serviceworker.js to assets/

f61211a

Signed-off-by: Andrew Thornton <art27@cantab.net>

zeripath added the type/bug label May 10, 2021

zeripath added this to the 1.15.0 milestone May 10, 2021

techknowlogick approved these changes May 10, 2021

View reviewed changes

GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label May 10, 2021

techknowlogick added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label May 10, 2021

silverwind reviewed May 10, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

add serviceworker.js back in to reserved usernames

dfd5ad5

Signed-off-by: Andrew Thornton <art27@cantab.net>

silverwind mentioned this pull request May 11, 2021

Unregister non-matching serviceworkers #15834

Merged

6543 closed this in #15834 May 12, 2021

zeripath deleted the redirect-serviceworker.js-to-assets branch May 12, 2021 19:26

go-gitea locked and limited conversation to collaborators Oct 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redirect /serviceworker.js to assets/ #15823

Redirect /serviceworker.js to assets/ #15823

zeripath commented May 10, 2021

silverwind commented May 10, 2021

silverwind May 10, 2021 •

edited

Loading

zeripath May 11, 2021

lunny May 11, 2021

silverwind May 11, 2021 •

edited

Loading

a1012112796 commented May 11, 2021

a1012112796 commented May 11, 2021

zeripath commented May 11, 2021

zeripath commented May 11, 2021

zeripath commented May 11, 2021

silverwind commented May 11, 2021 •

edited

Loading

silverwind commented May 11, 2021 •

edited

Loading

This comment has been minimized.

zeripath commented May 11, 2021

silverwind commented May 11, 2021 •

edited

Loading

codecov-commenter commented May 11, 2021

	http.Redirect(resp, req, setting.AppURL+"assets/serviceworker.js", http.StatusFound)
	http.Redirect(resp, req, setting.AppURL+"assets/serviceworker.js", http.StatusTemporaryRedirect)

Redirect /serviceworker.js to assets/ #15823

Redirect /serviceworker.js to assets/ #15823

Conversation

zeripath commented May 10, 2021

silverwind commented May 10, 2021

silverwind May 10, 2021 • edited Loading

Choose a reason for hiding this comment

zeripath May 11, 2021

Choose a reason for hiding this comment

lunny May 11, 2021

Choose a reason for hiding this comment

silverwind May 11, 2021 • edited Loading

Choose a reason for hiding this comment

a1012112796 commented May 11, 2021

a1012112796 commented May 11, 2021

zeripath commented May 11, 2021

zeripath commented May 11, 2021

zeripath commented May 11, 2021

silverwind commented May 11, 2021 • edited Loading

silverwind commented May 11, 2021 • edited Loading

This comment has been minimized.

zeripath commented May 11, 2021

silverwind commented May 11, 2021 • edited Loading

codecov-commenter commented May 11, 2021

Codecov Report

silverwind May 10, 2021 •

edited

Loading

silverwind May 11, 2021 •

edited

Loading

silverwind commented May 11, 2021 •

edited

Loading

silverwind commented May 11, 2021 •

edited

Loading

silverwind commented May 11, 2021 •

edited

Loading