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 static favicon #7676

Closed
wants to merge 2 commits into from
Closed

Add static favicon #7676

wants to merge 2 commits into from

Conversation

bt90
Copy link
Contributor

@bt90 bt90 commented May 14, 2021

Purpose

This PR adds a static favicon with the path /favicon.ico and exempts it from the authentication logic. This allows Bitwarden and probably other tools to pick up the favicon even if the web UI requires authentication.

TODO: modify index.html to first load the static favicon in order to solve #7638

Testing

Successfully loaded the favicon without logging in.

@bt90
Copy link
Contributor Author

bt90 commented May 16, 2021

Had a quick look at the HTML/JS code and decided not to touch it.

@AudriusButkevicius
Copy link
Member

AudriusButkevicius commented May 16, 2021

I feel that this could be made prettier. The fact that some list of files that we permit lives in the basic auth middleware sounds sad.

Perhaps something like:

authMiddleware := authMiddleware(handler)
handler = noAuthMiddleware(handler, authMiddleware, []string{"/noauth1", "/noauth2"})

@bt90
Copy link
Contributor Author

bt90 commented May 16, 2021

some list of files

Do we have any other files which should bypass authentication?

I feel that this could be made prettier.

While i agree that a separate method would be prettier, that's about the limit of what I dare contribute without go experience 😅

@greatroar
Copy link
Contributor

That's a pretty huge favicon. Any reason not to use the <5KiB one in cmd/ursrv/static/assets/img/favicon.png?

@bt90
Copy link
Contributor Author

bt90 commented May 17, 2021

That's a pretty huge favicon. Any reason not to use the <5KiB one in cmd/ursrv/static/assets/img/favicon.png?

Just reusing our current ico:

https://github.com/syncthing/syncthing/blob/main/assets/logo.ico

The ico format also bundles multiple resolutions.

@greatroar
Copy link
Contributor

Not a big deal (it compresses to 50KiB), it's just that all the other favicons are way smaller.

Regarding the Go code, the favicon skips the auth but does still send the device ID in the X-Syncthing-Id header.

@bt90
Copy link
Contributor Author

bt90 commented May 17, 2021

Regarding the Go code, the favicon skips the auth but does still send the device ID in the X-Syncthing-Id header.

Good catch. Probably better to do it like @AudriusButkevicius suggested and handle this in a separate function.

@greatroar
Copy link
Contributor

Inserting this between the IsAuthEnabled and UseTLS checks seems to work:

faviconMux := http.NewServeMux()
faviconMux.HandleFunc("/favicon.ico", func(w http.ResponseWriter, r *http.Request) {
	io.WriteString(w, "insert icon here")
})
faviconMux.Handle("/", handler)
handler = faviconMux

Maybe the statics handler can be put there?

@st-review
Copy link

🤖 beep boop

I'm going to close this pull request as it has been idle for more than 90 days.

This is not a rejection, merely a removal from the list of active pull requests that is periodically reviewed by humans. The pull request can be reopened when there is new activity, and merged once any remaining issues are resolved.

@st-review st-review closed this Aug 16, 2021
@bt90 bt90 mentioned this pull request Jul 5, 2022
11 tasks
@st-review st-review added the frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion label Mar 10, 2023
@syncthing syncthing locked and limited conversation to collaborators Mar 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants