-
-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Prevent Firefox from using apple-touch-icon #10402
Conversation
Codecov Report
@@ Coverage Diff @@
## master #10402 +/- ##
==========================================
- Coverage 43.7% 43.68% -0.02%
==========================================
Files 586 586
Lines 81354 81354
==========================================
- Hits 35552 35540 -12
- Misses 41401 41412 +11
- Partials 4401 4402 +1
Continue to review full report at Codecov.
|
routers/routes/routes.go
Outdated
@@ -1002,6 +1002,12 @@ func RegisterRoutes(m *macaron.Macaron) { | |||
} | |||
}) | |||
|
|||
m.Get("/apple-touch-icon.png", func(ctx *context.Context) { | |||
file := path.Join(setting.StaticRootPath, "public/img/apple-touch-icon.png") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not right because the file maybe embedded into binary. You should use functions from modules/public
to serve file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed it to a redirect. Ideally, it should be a "rewrite" but I'm not sure how I would do that. I would like to avoid having to set all the caching headers and such which seems to be needed with public.Asset
which only returns bytes without metadata.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Open to suggestions on how to delegate the request directly to the static file hander.
Maybe you should change appsuburl to StaticUrlPrefix so that it's possible to be serve by CDN. |
What's the difference between AppSubUrl and StaticUrlPrefix exactly? On a default installation, both are empty string and on gitea.com StaticUrlPrefix is on a second domain. Both seem to serve assets fine. |
@silverwind on gitea.com |
The opaque background does not work well in Firefox which uses the icon as a "rich icon". Prevent this by not specifying it in HTML. Real Apple devices will still request the icon on the static path. Fixes: go-gitea#10394 Also adjust gitignore so app.ini.sample becomes searchable and fixed a variable name in app.ini.sample.
Ok, using Pushed two additional tweaks:
|
gitignore fixed, it needs to be a bit verbose to work (taken from https://stackoverflow.com/a/16318111/808699). |
/custom/* | ||
!/custom/conf | ||
/custom/conf/* | ||
!/custom/conf/app.ini.sample |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why add those to .gitignore
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/custom/conf/app.ini.sample
is tracked in git so should not be in.gitignore
- so I can search
/custom/conf/app.ini.sample
usingag
orrg
tools which respect.gitignore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And no, these 4 lines can not be reduced to 2 because of how .gitignore
works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@silverwind so those lines are required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it will not work with just 2 lines, that was my first failed attempt.
Those 4 lines essentially say "exclude everything in /custom
except /custom/conf/app.ini.sample
"
@@ -217,7 +217,7 @@ FILE_EXTENSIONS = .md,.markdown,.mdown,.mkd | |||
PROTOCOL = http | |||
DOMAIN = localhost | |||
ROOT_URL = %(PROTOCOL)s://%(DOMAIN)s:%(HTTP_PORT)s/ | |||
; when STATIC_URL_PREFIX is empty it will follow APP_URL | |||
; when STATIC_URL_PREFIX is empty it will follow ROOT_URL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why following ROOT_URL
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ROOT_URL
is correct in config file context. The name AppURL
is only used in code:
AppURL = sec.Key("ROOT_URL").MustString(defaultAppURL)
I would say ready for merge? |
The opaque background does not work well in Firefox which uses the icon as a "rich icon". Prevent this by not specifying it in HTML. Real Apple devices will still request the icon on the static path.
Fixes: #10394