feat: require package maintainer permission when granting team package access#1027
feat: require package maintainer permission when granting team package access#1027
Conversation
…e access Non-admin users must be a maintainer of the package before they can add it to a team. This prevents team owners from granting access to packages they do not have permission over. Admin users are exempt. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an Organization and Team management system, including support for team-based package access control and version-level package blocking. The review identified several critical security and code quality issues, including the use of an insecure RSA key length, missing access control checks for tarball downloads, and the need for stronger type safety for the 'role' property in TeamMember entities.
I am having trouble creating individual review comments. Click here to see my feedback.
app/common/CryptoUtil.ts (7)
Using an RSA key with a modulus length of 512 bits is insecure and highly vulnerable to factorization attacks. It is strongly recommended to use a key length of at least 2048 bits for adequate security.
modulusLength: 2048,
app/port/controller/package/DownloadPackageVersionTar.ts (54-59)
The logic to check for blocked package versions appears to be missing. A user could potentially download the tarball of a blocked version. You should add checks for both package-level and version-level blocks before proceeding with the download. You can achieve this by calling this.getPackageEntity(scope, name) and this.getPackageVersionEntity(pkg, version).
const [ scope, name ] = getScopeAndName(fullname);
await this.userRoleManager.checkReadAccess(ctx, scope, name);
const version = this.getAndCheckVersionFromFilename(ctx, fullname, filenameWithVersion);
const pkg = await this.getPackageEntity(scope, name);
await this.getPackageVersionEntity(pkg, version);
const storeKey = "/packages/" + fullname + "/" + version + "/" + filenameWithVersion + ".tgz";
const downloadUrl = await this.nfsAdapter.getDownloadUrl(storeKey);
app/core/entity/TeamMember.ts (8)
The role property should be a specific union type 'owner' | 'member' instead of a generic string to ensure type safety and prevent invalid roles from being assigned. This aligns with the documentation and the implementation of OrgMember.
role: 'owner' | 'member';
app/core/entity/TeamMember.ts (17)
The role property should be a specific union type 'owner' | 'member' instead of a generic string to ensure type safety and prevent invalid roles from being assigned. This aligns with the documentation and the implementation of OrgMember.
role: 'owner' | 'member';
app/core/entity/TeamMember.ts (24)
With the role property now strongly typed as 'owner' | 'member', a valid role will always be provided upon TeamMember creation. The fallback to 'member' is no longer necessary and can be removed.
this.role = data.role;
app/repository/model/TeamMember.ts (28)
For better type safety and to align with the database schema comment which specifies 'owner or member', the type of role should be 'owner' | 'member' instead of a generic string.
role: 'owner' | 'member';
|
@codex review |
|
ci 不会跑了？ |
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…API (#1026) `EdgedriverBinary.fetch('/<version>/')` has been broken since around **2026-04-07** — the legacy Azure Blob container listing API now returns **HTTP 409 `PublicAccessNotPermitted`** after Microsoft disabled public access on the storage account: ``` https://msedgewebdriverstorage.blob.core.windows.net/edgewebdriver?prefix=<version>/&delimiter=/&comp=list → HTTP 409 PublicAccessNotPermitted ``` This made `test/common/adapter/binary/EdgedriverBinary.test.ts > fetch() > should work` fail on the `items.length >= 3` assertion, blocking `@eggjs/egg` monorepo's E2E pipeline and any project depending on EdgedriverBinary. Microsoft has migrated Edge WebDriver hosting to `https://msedgedriver.microsoft.com/`. Per-version files are still reachable at the same path pattern: ``` https://msedgedriver.microsoft.com/<version>/edgedriver_<platform>.zip ``` The new host **also** serves a `listing.json` dump (used by Microsoft's own catalog page at https://msedgedriver.microsoft.com/catalog/), but it has two drawbacks: 1. **It's a single static file containing every version since `112.0.1722.39`** — ~9000 entries, ~1.2MB. I verified empirically that **none of `?top` / `?limit` / `?max` / `?pageSize` / `?prefix` / `?latest` query parameters do anything** — the response is byte-identical regardless of query string. There is no server-side filtering. 2. Even with caching, that's 1.2MB extra per sync. So instead this PR mirrors the approach already used by [`FirefoxBinary`](https://github.com/cnpm/cnpmcore/blob/master/app/common/adapter/binary/FirefoxBinary.ts) and [`ChromeForTestingBinary`](https://github.com/cnpm/cnpmcore/blob/master/app/common/adapter/binary/ChromeForTestingBinary.ts): **don't call any listing API for sub-dirs at all.** Instead, enumerate the per-version download URLs from a static list of known platform filenames: ```ts const EDGEDRIVER_PLATFORM_FILES = [ 'edgedriver_arm64.zip', 'edgedriver_linux64.zip', 'edgedriver_mac64.zip', 'edgedriver_mac64_m1.zip', 'edgedriver_win32.zip', 'edgedriver_win64.zip', ] as const; ``` Each emitted `BinaryItem` carries `ignoreDownloadStatuses: [404]`, so any version that doesn't ship every platform (e.g. older builds without `mac64_m1`) is skipped cleanly by cnpmcore's sync pipeline rather than failing the task. The root-dir listing path (`#syncDirItems` → `https://edgeupdates.microsoft.com/api/products`) is **completely unchanged** — that JSON API still works. | | Use `listing.json` | Generate URLs (this PR) | |---|---|---| | Network per sub-dir fetch | ~1.2MB JSON dump | **0 bytes** | | Per-sync caching needed | yes (~1.2MB cached + reset logic) | no | | Failure modes | listing endpoint down → all sub-dir fetches fail | per-platform 404 handled cleanly | | Code surface | listing parse + cache + error path | one `.map()` over a const array | | Singleton state safety | needs careful reset in `initFetch` | none, stateless | | Platform discovery | from server response | static const (loses any net-new platform until updated) | The trade-off is that if Microsoft adds a new Edge WebDriver platform (e.g. `linux_arm64`), this adapter won't pick it up until the const list is updated. That's a maintenance cost but a small one — the platform list has been stable at six entries throughout the entire history of the new CDN. - **Root-dir listing (`fetch('/')`)**: still uses `https://edgeupdates.microsoft.com/api/products`. Microsoft only exposes the very latest release of each channel via this API, so the result today is ~5 versions total (one per channel: Stable / Beta / Dev / Canary). Historical version coverage was — and still is — governed by what cnpmcore had previously synced, not by what edgeupdates returns now. - **Version ordering in the root listing**: items come out in the order Microsoft's API returns them, which is grouped by channel (Stable → Beta → Dev → Canary), newest within each channel. This is *not* a strict latest-first sort, but it matches the pre-PR behavior. If a strict ordering is desired, that's a separate change. Microsoft's per-version download URL works for **versions from `112.0.1722.39` onwards**. Versions older than 112 are no longer downloadable from any public Microsoft endpoint: | Version | Old Blob URL | New URL | |---|---|---| | 80.0.361.111 | 409 PublicAccessNotPermitted | 404 | | 100.0.1185.27 | 409 PublicAccessNotPermitted | 404 | | 110.0.1587.69 | 409 PublicAccessNotPermitted | 404 | | **112.0.1722.39** | 409 PublicAccessNotPermitted | **200 ✓** | | 120.0.2210.133 | 409 PublicAccessNotPermitted | 200 ✓ | | 148.0.3966.0 | 409 PublicAccessNotPermitted | 200 ✓ | This is a Microsoft-side cutoff, not something cnpmcore can work around. **Existing mirrored copies of pre-112 Edge WebDriver binaries in cnpmcore's own storage are unaffected** — this PR only changes *discovery* of new files from upstream. Also adds `await binary.initFetch()` in the test's `beforeEach`. `EdgedriverBinary` is a `@SingletonProto`, so `app.getEggObject(EdgedriverBinary)` returns the same instance across tests, which means `dirItems` populated by one test would leak into the next. This was a pre-existing latent issue masked by the old test only having one `it` block, but it surfaces immediately when adding more cases. There is no formal Microsoft announcement for this specific storage lockdown, but the migration off `azureedge.net` and the gradual lockdown of the legacy XML listing have been documented across multiple community projects. Timeline: - **2024-05** — [MicrosoftEdge/EdgeWebDriver#146](MicrosoftEdge/EdgeWebDriver#146): Azure Blob listing stopped updating for versions ≥ 125.0. - **2025-01** — [MicrosoftEdge/EdgeWebDriver#183](MicrosoftEdge/EdgeWebDriver#183): `azureedge.net` CDN sunset (Edgio bankruptcy); Microsoft initially said it would continue under a different CDN provider. - **2025-07** — [seleniumbase/SeleniumBase#3888](seleniumbase/SeleniumBase#3888), [MicrosoftEdge/EdgeWebDriver#201](MicrosoftEdge/EdgeWebDriver#201), [#203](MicrosoftEdge/EdgeWebDriver#203): `msedgedriver.azureedge.net` DNS stopped resolving; new CDN is `msedgedriver.microsoft.com`. SeleniumBase fixed this in 4.40.6. - **2026-04-07** — (this PR): The Azure Blob container backing the old XML listing also had public access disabled; only `msedgedriver.microsoft.com` works now. - [x] `EdgedriverBinary.test.ts > should list recent stable versions from edgeupdates.microsoft.com` — green - [x] `EdgedriverBinary.test.ts > should generate all known platform driver URLs for a version` — green (asserts the exact 6 generated items with `ignoreDownloadStatuses: [404]` on each) - [x] **All 6 cnpmcore CI matrices** (`mysql node@22/24 × jsonBuilder true/false`, `postgresql node@22/24`) green - [x] `test-deployment`, `typecheck` (lint + fmtcheck + tsc), `codecov/patch`, `codecov/project` green - [ ] Manual smoke (suggested for reviewer): a fresh sync of a recent Edge WebDriver version (e.g. 148.0.3966.0) enumerates all 6 platform binaries and downloads all of them successfully, without 404s - [ ] Manual smoke (suggested for reviewer): a sync of an older version that doesn't ship every platform (e.g. 112.0.1722.39 has no `mac64_m1`) — the missing platform should be skipped via `ignoreDownloadStatuses: [404]` and the rest should complete 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> * **Bug Fixes** * Improved handling of unavailable EdgeDriver versions for specific platforms—missing combinations are now skipped instead of causing failures. * **Improvements** * Enhanced EdgeDriver binary delivery infrastructure for greater reliability and consistency. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
需要同步一下 |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 3.x #1027 +/- ##
==========================================
- Coverage 96.22% 96.19% -0.03%
==========================================
Files 208 208
Lines 21018 20932 -86
Branches 2851 2848 -3
==========================================
- Hits 20224 20135 -89
- Misses 794 797 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
🎉 This PR is included in version 3.82.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
2 participants
Summary
Test plan
🤖 Generated with Claude Code