Security & release prep 0.1.10: CSRF hardening, deps, PHP 8.2 lock, lint#25
Merged
Conversation
Security: - Remove @NoCSRFRequired from query/queryUser/register/update/remove/ registerState/removeUser. These are all called from the app's own UI via @nextcloud/axios, which always sends the CSRF token, so the annotation was unnecessary and left state-changing admin/user endpoints (register/update/ remove, removeUser) open to CSRF. callback() keeps it -- it is the external OIDC redirect target and cannot carry a token (the state lookup guards it). - npm audit fix: resolves all 4 production advisories (2 high), including the axios prototype-pollution/SSRF chain that carries the OIDC tokens. Production dependencies now report 0 vulnerabilities (remaining 9 are dev-only). Correctness / release: - Remove the dead api#query_state route (no queryState controller method exists; nothing calls it). - Pin config.platform.php to 8.2 and re-resolve composer.lock. The lock was generated on PHP 8.4 and pinned php-cs-fixer deps (symfony/* v8, sebastian/diff 8) requiring PHP >=8.4, so `composer install` -- and the php-cs CI job -- would fail on the declared minimum PHP 8.2. Bump version to 0.1.10.
The lint/stylelint npm scripts existed but neither tool nor config was installed, so they could not run. Add @nextcloud/eslint-config (flat config) and @nextcloud/stylelint-config, mirror edusign's baseline config, and add a js-lint CI job (ESLint + stylelint) alongside php-cs. lint:fix standardised PersonalSettings.vue / IOIDCIcon.vue indentation and attribute order to match AdminSettings.vue (mechanical, no logic change). camelcase / eqeqeq / no-useless-assignment are relaxed as a baseline to avoid risky changes to existing OIDC flow code; tighten incrementally.
There was a problem hiding this comment.
Pull request overview
This PR prepares the Nextcloud Integration OIDC app for the 0.1.10 release by hardening CSRF protections on controller endpoints, updating dependency locks for PHP 8.2 compatibility, and introducing JS/CSS linting tooling and CI checks.
Changes:
- Removed
@NoCSRFRequiredfrom multipleApiControllerendpoints (keeping it for the external OIDCcallback()). - Added ESLint + stylelint configuration and a new GitHub Actions
js-lintjob. - Updated
composer.lock/composer.jsonto resolve dependencies against a PHP 8.2 platform target; bumped app version to 0.1.10.
Reviewed changes
Copilot reviewed 11 out of 13 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| stylelint.config.js | Adds shared Nextcloud stylelint configuration entrypoint. |
| eslint.config.mjs | Adds ESLint flat config using @nextcloud/eslint-config with baseline rule relaxations. |
| .github/workflows/lint-php-cs.yml | Adds js-lint job (ESLint + stylelint) alongside existing PHP CS checks. |
| package.json | Adds Nextcloud lint config packages to devDependencies (tooling enablement). |
| src/components/AdminSettings.vue | Mechanical formatting + prop casing updates; touched template semantics and script formatting. |
| src/components/PersonalSettings.vue | Mechanical formatting + prop casing updates; touched template semantics and script formatting. |
| src/components/icons/IOIDCIcon.vue | Formatting-only change in template/script. |
| lib/Controller/ApiController.php | Removes @NoCSRFRequired from non-callback endpoints for CSRF hardening. |
| appinfo/routes.php | Removes dead api#query_state route. |
| composer.json | Pins Composer platform PHP version to 8.2 for lock compatibility. |
| composer.lock | Re-resolves lock for PHP 8.2-compatible dependency set. |
| appinfo/info.xml | Bumps app version from 0.1.9 to 0.1.10. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <option id="Google" required value="Google"> | ||
| </option> | ||
| <option id="Microsoft" required value="Microsoft" /> |
| @default="populate"> | ||
| <p> | ||
| Explanations for the parameters are | ||
| <a :href="documentation_link" target="_blank">documented here</a>. |
| :disabled="!isValid" | ||
| :readonly="readonly" | ||
| :wide="true" | ||
| :nativeType="submit" |
Comment on lines
+318
to
324
| mounted() { | ||
| const url = generateUrl('/apps/integration_oidc/query') | ||
| axios.get(url).then((result) => (this.configured = result.data)) | ||
| }, | ||
|
|
||
| methods: { | ||
| async set_ms_urls() { |
| :disabled="false" | ||
| :readonly="readonly" | ||
| :wide="true" | ||
| :nativeType="submit" |
Comment on lines
+83
to
88
| mounted() { | ||
| this.private_load().then(() => {}) | ||
| }, | ||
|
|
||
| methods: { | ||
| async private_load() { |
| console.log('available', this.available) | ||
| url = generateUrl('/apps/integration_oidc/query_user') | ||
| result = await axios.get(url) | ||
| // The configuired providers for this user |
- Add rel="noopener noreferrer" to the target="_blank" documentation link (prevents reverse tabnabbing). - Pass nativeType as a string literal instead of binding the undefined `submit` identifier (AdminSettings + PersonalSettings). - Give the Microsoft provider <option> a visible label (was self-closed and rendered blank). - Add the populate() method both components reference via @default and call it from mounted() so both load paths stay in sync. - Fix "configuired" typo in a comment.
Member
Author
|
Thanks — addressed all 7 review comments in 4ccb5db (these were pre-existing issues my reformat surfaced; fixing them for the release):
ESLint, stylelint, php-cs and the build all pass locally. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Security & release prep for 0.1.10
CSRF hardening (
3831258)Removed
@NoCSRFRequiredfromquery,queryUser,register,update,remove,registerState,removeUser. All are called from the app's own UI via@nextcloud/axios(which always sends the CSRF token), so the annotation was unnecessary and left state-changing admin/user endpoints (register/update/remove,removeUser) open to CSRF.callback()keeps it — it is the external OIDC redirect target and cannot carry a token (thestatelookup is its guard).Dependency security
npm audit fix(non-breaking) resolves all 4 production advisories (2 high), including theaxiosprototype-pollution / SSRF chain that carries the OIDC tokens. Production dependencies now report 0 vulnerabilities (the remaining 9 are dev-only build tooling).PHP 8.2 lock compatibility (fixes red CI)
The
composer.lockwas generated on PHP 8.4 and pinned php-cs-fixer's deps (symfony/* v8,sebastian/diff 8) to versions requiring PHP ≥8.4, socomposer install— and thephp-csCI job — would fail on the app's declared minimum PHP 8.2. Pinnedconfig.platform.phpto 8.2 and re-resolved.Cleanup
api#query_stateroute (noqueryStatecontroller method exists; nothing calls it).Tooling (
8157369)@nextcloud/eslint-config, flat config) + stylelint config — the npm scripts existed but neither tool was installed.js-lintCI job (ESLint + stylelint) alongsidephp-cs.lint:fixstandardisedPersonalSettings.vue/IOIDCIcon.vueformatting to matchAdminSettings.vue(mechanical, no logic change).camelcase/eqeqeq/no-useless-assignmentrelaxed as a baseline to avoid risky edits to OIDC flow code.Version bumped to 0.1.10. Build passes; ESLint, stylelint, php-cs all green locally.