-
Notifications
You must be signed in to change notification settings - Fork 622
Moves JSON-LD changes from old repo to new repo #5206
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
Conversation
| // Verify social profiles | ||
| assert(Array.isArray(orgSchema.sameAs), 'Organization should have sameAs array'); | ||
| assert.strictEqual(orgSchema.sameAs.length, 2); | ||
| assert(orgSchema.sameAs.includes('https://twitter.com/examplecorp')); |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High test
https://twitter.com/examplecorp
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
To fix the problem, the test should check that the actual value(s) in orgSchema.sameAs array match the required host(s), not simply that the specified URL appears as a substring.
Specifically, replace orgSchema.sameAs.includes('https://twitter.com/examplecorp') with a check that at least one value in the array, when parsed as a URL, has a host that matches 'twitter.com' and a pathname that matches the expected path ('/examplecorp'). Use Node's built-in url module (or the global URL class, if available) to parse each entry before comparison.
Edit only the relevant assertion on line 118, using Array.some together with URL parsing.
Since the test uses NodeJS, we can use the global URL class without additional imports.
-
Copy modified lines R118-R128
| @@ -115,7 +115,17 @@ | ||
| // Verify social profiles | ||
| assert(Array.isArray(orgSchema.sameAs), 'Organization should have sameAs array'); | ||
| assert.strictEqual(orgSchema.sameAs.length, 2); | ||
| assert(orgSchema.sameAs.includes('https://twitter.com/examplecorp')); | ||
| assert( | ||
| orgSchema.sameAs.some(u => { | ||
| try { | ||
| const { host, pathname } = new URL(u); | ||
| return host === 'twitter.com' && pathname === '/examplecorp'; | ||
| } catch (e) { | ||
| return false; | ||
| } | ||
| }), | ||
| 'Organization should have a Twitter "sameAs" with correct host and pathname' | ||
| ); | ||
| assert(orgSchema.sameAs.includes('https://linkedin.com/company/example')); | ||
| }); | ||
|
|
| assert(Array.isArray(orgSchema.sameAs), 'Organization should have sameAs array'); | ||
| assert.strictEqual(orgSchema.sameAs.length, 2); | ||
| assert(orgSchema.sameAs.includes('https://twitter.com/examplecorp')); | ||
| assert(orgSchema.sameAs.includes('https://linkedin.com/company/example')); |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High test
https://linkedin.com/company/example
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
The best way to fix this problem is to avoid relying on substring (includes) checks and instead use exact string matching for URLs within the sameAs array. This involves replacing the substring check (orgSchema.sameAs.includes(...)) with a check that at least one string in the array is exactly equal to the expected URL. In JavaScript, this can be done with the Array.prototype.some() method, for example: orgSchema.sameAs.some(url => url === 'https://linkedin.com/company/example'). This ensures that only the exact expected URL passes the check, eliminating the risk of dangerous substrings. The required change is localized to line 119 in the test file, with no imports or additional definitions required.
-
Copy modified line R119
| @@ -116,7 +116,7 @@ | ||
| assert(Array.isArray(orgSchema.sameAs), 'Organization should have sameAs array'); | ||
| assert.strictEqual(orgSchema.sameAs.length, 2); | ||
| assert(orgSchema.sameAs.includes('https://twitter.com/examplecorp')); | ||
| assert(orgSchema.sameAs.includes('https://linkedin.com/company/example')); | ||
| assert(orgSchema.sameAs.some(url => url === 'https://linkedin.com/company/example')); | ||
| }); | ||
|
|
||
| it('should render homepage with minimal configuration', async function () { |
| llmsTxt.includes('A test site demonstrating llms.txt'), | ||
| 'Should include site description' | ||
| ); | ||
| assert(llmsTxt.includes('https://example.com'), 'Should include base URL'); |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High test
https://example.com
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
To fix the problem, we should avoid substring matching when verifying that a particular canonical URL appears in the llms.txt content in a security-sensitive or correctness-sensitive way. Instead, we should parse the output: extract lines that contain URLs (e.g. by matching lines containing https://), and properly parse the URLs using Node's url or URL class to confirm that 'example.com' is indeed the host of a listed URL. For this test, the optimal fix is to look for a line/lines in llms.txt containing a URL, extract the URL using a regex, and parse it to check that the host is example.com. All required code edits should be limited to the single test block containing line 463. An extra import is not strictly required, as Node.js has a built-in URL class.
-
Copy modified lines R463-R472
| @@ -460,7 +460,16 @@ | ||
| llmsTxt.includes('A test site demonstrating llms.txt'), | ||
| 'Should include site description' | ||
| ); | ||
| assert(llmsTxt.includes('https://example.com'), 'Should include base URL'); | ||
| // Look for a line with a URL, extract it, and check host | ||
| const urlMatches = llmsTxt.match(/https?:\/\/[^\s)]+/g) || []; | ||
| const hasExampleCom = urlMatches.some(u => { | ||
| try { | ||
| return (new URL(u)).hostname === 'example.com'; | ||
| } catch (e) { | ||
| return false; | ||
| } | ||
| }); | ||
| assert(hasExampleCom, 'Should include base URL with host example.com'); | ||
| assert( | ||
| llmsTxt.includes('## AI Training Policy') || | ||
| llmsTxt.includes('AI Training'), |
ValJed
left a comment
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.
There are too many unwanted changes in the package.json
packages/seo/package.json
Outdated
| "url": "git+https://github.com/apostrophecms/seo.git" | ||
| }, | ||
| "homepage": "https://github.com/apostrophecms/apostrophe/tree/main/packages/seo", | ||
| "homepage": "https://github.com/apostrophecms/seo#readme", |
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.
You should reference inside the monorepo since the seo module will die.
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, along with the "repository"
| "lodash": "^4.17.21" | ||
| }, | ||
| "devDependencies": { | ||
| "eslint": "^9.39.1", |
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.
you should keep eslint dep
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.
Not sure how that got removed. Added back.
packages/seo/package.json
Outdated
| "@apostrophecms/blog": "^1.0.6", | ||
| "@apostrophecms/seo": "file:.", | ||
| "apostrophe": "^4.24.0", | ||
| "eslint-config-apostrophe": "^5.0.0", |
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.
"eslint-config-apostrophe": "workspace:^",
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.
packages/seo/package.json
Outdated
| "eslint-config-apostrophe": "workspace:*" | ||
| "@apostrophecms/blog": "^1.0.6", | ||
| "@apostrophecms/seo": "file:.", | ||
| "apostrophe": "^4.24.0", |
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.
"apostrophe": "workspace:^",
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.
packages/seo/package.json
Outdated
| "eslint": "^9.39.1", | ||
| "eslint-config-apostrophe": "workspace:*" | ||
| "@apostrophecms/blog": "^1.0.6", | ||
| "@apostrophecms/seo": "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.
Do we need that?
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, for the functional tests
packages/seo/package.json
Outdated
| "devDependencies": { | ||
| "eslint": "^9.39.1", | ||
| "eslint-config-apostrophe": "workspace:*" | ||
| "@apostrophecms/blog": "^1.0.6", |
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.
"@apostrophecms/blog": "workspace:^",
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
boutell
left a comment
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.
LGTM, I must admit there's too much here to go line by line.
boutell
left a comment
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.
OK it sounds like this is what I reviewed before, just migrating.
packages/seo/.changeset/config.json
Outdated
| @@ -0,0 +1,11 @@ | |||
| { | |||
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.
You should run pnpm changeset in the repo root, not in individual package folders. Note the existing root .changeset folder. The individual changeset files know which package they impact.
boutell@Mac:~/apostrophecms/apostrophe$ ls .changeset/
config.json modern-radios-attack.md nice-peas-care.md README.md soft-laws-doubt.md
boutell@Mac:~/apostrophecms/apostrophe$ ff changeset
./.changeset/modern-radios-attack.md
./.changeset/config.json
./.changeset/nice-peas-care.md
./.changeset/README.md
./.changeset/soft-laws-doubt.md
Summary
Summarize the changes briefly, including which issue/ticket this resolves. If it closes an existing Github issue, include "Closes #[issue number]"
This PR moves the approved JSON-LD changes from the old repo to the new repo with no additional changes.
What are the specific steps to test this change?
For example:
What kind of change does this PR introduce?
(Check at least one)
Make sure the PR fulfills these requirements:
If adding a new feature without an already open issue, it's best to open a feature request issue first and wait for approval before working on it.
Other information: