-
Notifications
You must be signed in to change notification settings - Fork 9.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
added short-name-length check #145
Conversation
nice work here @samthor |
Would you mind adding this audit to one of the aggregators? :) |
Nice!!
Yah it'd work with the addto home screen aggregator.
|
a5194c5
to
c264f72
Compare
Added. PTAL! |
champ 👏 ⚾ 🎉 |
const manifest = artifacts.manifest.value; | ||
|
||
if (manifest && manifest.short_name) { | ||
// Historically, Chrome recommended 12 chars as the maximum length to prevent truncation. |
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.
maybe put in a mention of #69 to track while we figure out the answer there
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.
+1 lets link the issue as theres some valuable discussion.
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.
Done
72900c4
to
b0d756e
Compare
@paulirish Thanks for the explanation of the manifest vs parsed version. I get the approach now. PTAL |
const manifestSrc = JSON.stringify({ | ||
short_name: 'i\'m much longer than the recommended size' | ||
}); | ||
const manifest = manifestParser(manifestSrc); |
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.
We should also verify the Debug string is attached here
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.
Done
Lgtm % nit |
Let's land this after deep's scheduler. (We'll have a conflict to fix) |
b0d756e
to
0fb66b3
Compare
Cool, I can keep rebasing as long as you like 👍 |
Ah! looks like no conflicts. :) Not bad. Merging. |
Starts to solve #69 with a basic assumption of 12 chars max, taken from the old Chrome extension manifest docs (which presumably evolved into the format we have today).
(Also partially just learning how to write an audit).