ADLS Gen 2 - #2635
Conversation
|
@microsoft-github-policy-service agree |
|
hmm ok, just found this doc here: https://github.com/Azure/Azurite/wiki/ADLS-Gen2-Implementation-Guidance |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Adds native ADLS Gen2 (DFS) endpoint support to Azurite by standing up a first-class DFS server that shares the existing blob metadata/extent stores, plus a broad test suite and configuration/documentation updates.
Changes:
- Introduces a native DFS Express pipeline (context/auth/dispatch + filesystem/path handlers) running on a new configurable DFS host/port.
- Extends blob metadata stores with atomic rename operations to support DFS rename semantics (single-path + prefix/directory).
- Adds DFS-focused integration tests and updates CLI/VSC/Docker/README to expose the DFS endpoint and HNS toggle.
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/blob/dfsProxy.test.ts | Adds DFS end-to-end tests for filesystem/path ops, append/flush, ACLs, leases, and rename. |
| src/common/VSCEnvironment.ts | Adds dfsHost/dfsPort and enableHierarchicalNamespace to VS Code environment config. |
| src/common/Telemetry.ts | Adds dfsHost/dfsPort to telemetry parameter collection. |
| src/common/Environment.ts | Adds CLI flags for DFS host/port and hierarchical namespace toggle. |
| src/blob/utils/constants.ts | Adds DFS defaults and changes HNS default constant behavior. |
| src/blob/persistence/SqlBlobMetadataStore.ts | Implements SQL rename operations for DFS (single blob + prefix). |
| src/blob/persistence/LokiBlobMetadataStore.ts | Implements Loki rename operations for DFS (single blob + prefix). |
| src/blob/persistence/IBlobMetadataStore.ts | Extends store interface with rename methods used by DFS rename. |
| src/blob/main.ts | Starts DFS server alongside standalone blob server and wires shared stores. |
| src/blob/dfs/handlers/PathHandler.ts | Implements DFS path CRUD, list paths, append/flush, ACLs, leases, and rename. |
| src/blob/dfs/handlers/FilesystemHandler.ts | Implements DFS filesystem CRUD/list and property setting via container operations. |
| src/blob/dfs/DfsPropertyEncoding.ts | Adds shared base64 properties header encode/decode helper. |
| src/blob/dfs/DfsOperation.ts | Defines DFS operation enum for routing/auth mapping. |
| src/blob/dfs/DfsErrorFactory.ts | Adds DFS JSON error shaping (including special-casing HEAD). |
| src/blob/dfs/DfsContextFactory.ts | Creates minimal blob Context objects for store calls from DFS handlers. |
| src/blob/dfs/DfsContext.ts | Extracts DFS request context (account/filesystem/path) and performs version checks. |
| src/blob/dfs/DfsAuthenticationMiddleware.ts | Adds DFS auth middleware reusing blob authenticators. |
| src/blob/SqlBlobServer.ts | Exposes stores publicly to allow DFS to share them. |
| src/blob/IBlobEnvironment.ts | Extends blob environment interface with DFS host/port + HNS toggle. |
| src/blob/DfsServer.ts | Adds new DFS server implementation (HTTP/HTTPS) using shared stores. |
| src/blob/DfsRequestListenerFactory.ts | Adds DFS Express pipeline and operation dispatch. |
| src/blob/DfsConfiguration.ts | Adds DFS configuration wrapper based on existing ConfigurationBase. |
| src/blob/BlobServer.ts | Exposes stores publicly to allow DFS to share them. |
| src/blob/BlobEnvironment.ts | Adds blob-service CLI flags for DFS host/port and HNS toggle. |
| src/azurite.ts | Starts DFS server alongside blob/queue/table in the main Azurite entrypoint. |
| package.json | Adds VS Code extension config entries for dfsHost/dfsPort. |
| docs/designs/ADLS-gen2-parity.md | Documents the ADLS Gen2 parity approach, phases, and design rationale. |
| README.md | Documents DFS host/port usage and docker port exposure. |
| Dockerfile.Windows | Exposes DFS port and starts Azurite with dfsHost binding. |
| Dockerfile | Exposes DFS port and starts Azurite with dfsHost binding. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I added new commits to comply to the https://github.com/Azure/Azurite/wiki/ADLS-Gen2-Implementation-Guidance. |
There was a problem hiding this comment.
Pull request overview
This PR adds native ADLS Gen2 (DFS) endpoint support to Azurite by introducing a DFS server pipeline that shares the Blob service’s metadata/extent/account stores, along with HNS directory semantics and initial ACL enforcement.
Changes:
- Introduces a native DFS server (port 10004 by default) with filesystem/path handlers, auth middleware reuse, and DFS-specific context/error handling.
- Extends metadata stores (Loki + SQL) to support rename operations and an HNS hierarchy registry.
- Adds DFS-focused tests (proxy-style REST tests, SDK integration tests, and ACL enforcer unit tests) plus swagger artifacts/docs.
Reviewed changes
Copilot reviewed 45 out of 46 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/blob/dfsSDKIntegration.test.ts | Adds JS SDK integration coverage via @azure/storage-file-datalake. |
| tests/blob/dfsProxy.test.ts | Adds direct DFS REST-level tests and cross-API (Blob↔DFS) checks. |
| tests/blob/dfsAclEnforcer.test.ts | Adds unit tests for ACL parsing/evaluation logic. |
| swagger/dfs.md | Adds AutoRest configuration for DFS server generation. |
| swagger/dfs-storage-2023-11-03.json | Adds DFS swagger spec used for generated artifacts. |
| src/common/models.ts | Extends OAuth level enum with ACL enforcement mode. |
| src/common/VSCEnvironment.ts | Adds VS Code settings for DFS host/port + HNS toggle. |
| src/common/Telemetry.ts | Adds dfsHost/dfsPort telemetry parameter handling. |
| src/common/Environment.ts | Adds CLI flags for dfsHost/dfsPort and HNS enablement. |
| src/common/ConfigurationBase.ts | Parses new --oauth acl option. |
| src/blob/utils/constants.ts | Adds DFS defaults + flips HNS “enabled” constant default. |
| src/blob/persistence/SqlBlobMetadataStore.ts | Adds HNS hierarchy table + rename/HNS operations in SQL store. |
| src/blob/persistence/LokiBlobMetadataStore.ts | Adds rename/HNS operations and HNS collection in Loki store. |
| src/blob/persistence/IBlobMetadataStore.ts | Extends store interface with rename + HNS hierarchy methods. |
| src/blob/main.ts | Starts DFS server alongside blob server in azurite-blob. |
| src/blob/generated-dfs/handlers/handlerMappers.ts | Adds generated DFS handler mapper wiring. |
| src/blob/generated-dfs/handlers/IPathHandler.ts | Adds generated DFS path handler interface. |
| src/blob/generated-dfs/handlers/IHandlers.ts | Adds generated DFS handler registry interface. |
| src/blob/generated-dfs/handlers/IFilesystemHandler.ts | Adds generated DFS filesystem handler interface. |
| src/blob/generated-dfs/artifacts/specifications.ts | Adds operation matching specs for DFS routing. |
| src/blob/generated-dfs/artifacts/operation.ts | Adds generated DFS operation enum. |
| src/blob/generated-dfs/artifacts/models.ts | Adds generated DFS request/response model types. |
| src/blob/generated-dfs/Context.ts | Adds DFS-specific generated context wrapper. |
| src/blob/dfs/handlers/PathHandler.ts | Implements core DFS path operations (create/read/delete/list/update/lease/rename). |
| src/blob/dfs/handlers/FilesystemHandler.ts | Implements DFS filesystem ops (create/delete/properties/list/setProperties). |
| src/blob/dfs/DfsPropertyEncoding.ts | Adds helper for DFS x-ms-properties encoding/decoding. |
| src/blob/dfs/DfsOperation.ts | Adds DFS operation enum for middleware/dispatch. |
| src/blob/dfs/DfsErrorFactory.ts | Adds DFS JSON error writer + common DFS error types. |
| src/blob/dfs/DfsContextFactory.ts | Adds helper to create minimal Blob Context for store calls. |
| src/blob/dfs/DfsContext.ts | Adds DFS context middleware and URL parsing. |
| src/blob/dfs/DfsAuthenticationMiddleware.ts | Reuses blob authenticators for DFS + extracts identity for ACL mode. |
| src/blob/dfs/DfsAclEnforcer.ts | Adds ACL parsing and evaluation logic for DFS operations. |
| src/blob/SqlBlobServer.ts | Exposes stores publicly for DFS server sharing. |
| src/blob/IBlobEnvironment.ts | Extends blob environment interface with DFS host/port + HNS toggle. |
| src/blob/DfsServer.ts | Adds DFS server wrapper around request listener pipeline. |
| src/blob/DfsRequestListenerFactory.ts | Adds Express pipeline for DFS routing/auth/handlers. |
| src/blob/DfsConfiguration.ts | Adds configuration class for DFS endpoint. |
| src/blob/BlobServer.ts | Exposes stores publicly for DFS server sharing. |
| src/blob/BlobEnvironment.ts | Adds CLI flags for DFS host/port and HNS toggle in azurite-blob. |
| src/azurite.ts | Starts DFS server alongside blob/queue/table in the combined entrypoint. |
| package.json | Adds @azure/storage-file-datalake dev dependency and DFS extension settings/script. |
| docs/designs/ADLS-gen2-parity.md | Adds design/phase plan for DFS parity implementation. |
| README.md | Documents DFS host/port config and Docker port exposure. |
| Dockerfile.Windows | Exposes 10004 and passes --dfsHost. |
| Dockerfile | Exposes 10004 and passes --dfsHost. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I've updated the unit tests on this PR: OrchardCMS/OrchardCore#19014 |
|
Copilot open a new pull request to apply changes based on the comments in this thread |
There was a problem hiding this comment.
Pull request overview
Adds native ADLS Gen2 (DFS) endpoint support to Azurite by introducing a dedicated DFS server/pipeline that shares the existing Blob metadata + extent stores, plus new persistence helpers (rename + HNS hierarchy) and a broad set of DFS-focused tests and swagger artifacts.
Changes:
- Introduce a DFS server (port 10004 by default) with native Express handlers for filesystem/path operations, append/flush, leases, rename, and ACL metadata.
- Extend metadata stores (Loki + SQL) with rename APIs and HNS hierarchy tracking.
- Add DFS integration/proxy/unit tests, swagger inputs/config, and environment/Docker/README wiring for DFS.
Reviewed changes
Copilot reviewed 45 out of 46 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/blob/dfsSDKIntegration.test.ts | New JS SDK integration tests using @azure/storage-file-datalake. |
| tests/blob/dfsProxy.test.ts | New REST-level DFS behavior tests (create/list/append/flush/rename/ACL/leases). |
| tests/blob/dfsAclEnforcer.test.ts | Unit tests for ACL parsing/evaluation. |
| swagger/dfs.md | AutoRest config for DFS codegen. |
| swagger/dfs-storage-2023-11-03.json | DFS swagger used for codegen/artifacts. |
| src/common/models.ts | Add OAuthLevel.ACL enum value. |
| src/common/VSCEnvironment.ts | Add dfsHost/dfsPort + enableHierarchicalNamespace settings. |
| src/common/Telemetry.ts | Include DFS flags in telemetry parameter list. |
| src/common/Environment.ts | Add --dfsHost/--dfsPort and --enableHierarchicalNamespace CLI flags. |
| src/common/ConfigurationBase.ts | Parse --oauth acl into OAuthLevel.ACL. |
| src/blob/utils/constants.ts | Add DFS defaults; switch emulator HNS default to enabled. |
| src/blob/persistence/SqlBlobMetadataStore.ts | Add HNS hierarchy table + rename helpers. |
| src/blob/persistence/LokiBlobMetadataStore.ts | Add HNS hierarchy collection + rename helpers. |
| src/blob/persistence/IBlobMetadataStore.ts | Add interfaces for rename and HNS hierarchy operations. |
| src/blob/main.ts | Start DFS server alongside blob for azurite-blob. |
| src/blob/generated-dfs/** | New generated DFS artifacts/interfaces/specs. |
| src/blob/dfs/** | New DFS middleware, handlers, ACL enforcer, errors, property encoding. |
| src/blob/SqlBlobServer.ts | Expose stores publicly for DFS server sharing. |
| src/blob/IBlobEnvironment.ts | Add DFS + HNS configuration surface. |
| src/blob/DfsServer.ts | New DFS server wrapper over ServerBase. |
| src/blob/DfsRequestListenerFactory.ts | New DFS Express pipeline (context/dispatch/auth/routing/errors). |
| src/blob/DfsConfiguration.ts | New DFS configuration class. |
| src/blob/BlobServer.ts | Expose stores publicly for DFS server sharing. |
| src/blob/BlobEnvironment.ts | Add DFS host/port + HNS flag for azurite-blob. |
| src/azurite.ts | Start DFS server alongside blob/queue/table for azurite. |
| package.json | Add DFS autorest script + @azure/storage-file-datalake dep + VS Code settings for DFS host/port. |
| docs/designs/ADLS-gen2-parity.md | Design/phase plan for ADLS Gen2 parity implementation. |
| README.md | Document DFS host/port + Docker port mapping updates. |
| Dockerfile.Windows | Expose port 10004 and pass --dfsHost. |
| Dockerfile | Expose port 10004 and pass --dfsHost. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Copilot open a new pull request to apply changes based on the comments in this thread |
|
I'm sorry, I probably don't have the permission to run agents on this repository. |
|
No feedback on this one yet after 2 weeks. I know that this is AI generated and this is probably not optimal but at least some feedback would be welcomed. Should I simply drop the PR and consider simply keeping this on my own fork? Sébastien Ros (@sebastienros) sorry to ping you on this one. Totally non-related with OC but what's your thoughts on this? This is an open sourced project by Microsoft on the Azure repository. Not sure who's managing this or if I will ever get feedback at this point. Thanks. |
|
Hi Jasmin Savard (@Skrypt), thank you for the effort, I came across this PR while testing file system implementation for Apache Flink and on behavioural suits it has largely worked apart from a few things I have found with Claude. Please see the summary of Claude findings below: Title: DFS endpoint: three issues found testing with Azure Java SDK While testing a filesystem implementation for Apache Flink against this PR's DFS endpoint using the Azure Storage File DataLake Java SDK (v12.22.0), we found three issues. Core operations (create, append, flush, read, delete, list paths) all work well. 1. Rename fails for paths with The rename handler splits Fix: 2. The DFS handler correctly returns The DFS Workaround: emit metadata as 3. The SDK's Fix: add a dispatch rule for |
|
Great thanks for the review. I will take time and fix these when I have some time. |
|
Aleksandr Iushmanov (@Izeren) This should be fixed now. Let me know if anything. |
There was a problem hiding this comment.
Pull request overview
Adds ADLS Gen2 (DFS) parity work to Azurite by introducing a native DFS request pipeline and handlers that operate directly on the existing blob metadata/extent stores, along with supporting persistence changes, config wiring, and tests.
Changes:
- Introduces a DFS Express pipeline (context/dispatch/auth/HNS validation) and DFS handlers for filesystem + path operations (create/delete/read/list/append+flush/rename/ACL/lease).
- Extends Loki/SQL metadata stores with rename operations and an HNS hierarchy registry to support directory semantics.
- Adds Swagger artifacts, documentation, Docker/README updates, and new tests (SDK integration, proxy-style REST tests, ACL unit tests).
Reviewed changes
Copilot reviewed 52 out of 54 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/blob/dfsSDKIntegration.test.ts | Adds ADLS Gen2 SDK-level integration coverage via @azure/storage-file-datalake. |
| tests/blob/dfsProxy.test.ts | Adds REST-level DFS behavior tests (SAS, paths, ACLs, leases, rename, etc.). |
| tests/blob/dfsAclEnforcer.test.ts | Adds unit tests for ACL parsing/enforcement logic. |
| swagger/dfs.md | AutoRest configuration for DFS server generation. |
| swagger/dfs-storage-2023-11-03.json | DFS Swagger spec used for generated artifacts. |
| src/common/models.ts | Adds OAuth ACL level. |
| src/common/VSCEnvironment.ts | Adds DFS host/port and HNS enablement config hooks for VS Code environment. |
| src/common/Telemetry.ts | Tracks DFS port in telemetry filtering and adds DFS flags to parameter list. |
| src/common/Environment.ts | Adds CLI flags for DFS host/port and enabling HNS. |
| src/common/ConfigurationBase.ts | Parses --oauth acl. |
| src/blob/utils/constants.ts | Adds DFS defaults and flips HNS default constant behavior. |
| src/blob/persistence/SqlBlobMetadataStore.ts | Adds SQL HNS hierarchy model + rename/HNS registry methods. |
| src/blob/persistence/LokiBlobMetadataStore.ts | Adds Loki HNS hierarchy collection + rename/HNS registry methods. |
| src/blob/persistence/IBlobMetadataStore.ts | Extends the store interface for rename + HNS hierarchy methods. |
| src/blob/main.ts | Updates standalone blob entrypoint messaging and shutdown typing. |
| src/blob/handlers/ServiceHandler.ts | Wires HNS enablement into service GetAccountInfo response. |
| src/blob/handlers/ContainerHandler.ts | Persists per-container HNS marker and returns it on account info. |
| src/blob/handlers/BlobHandler.ts | Returns HNS marker on blob GetAccountInfo responses. |
| src/blob/generated/artifacts/models.ts | Adds isHierarchicalNamespaceEnabled to generated blob models. |
| src/blob/generated-dfs/handlers/handlerMappers.ts | Generated DFS operation→handler mapping. |
| src/blob/generated-dfs/handlers/IPathHandler.ts | Generated DFS path handler interface. |
| src/blob/generated-dfs/handlers/IHandlers.ts | Generated DFS handler registry interface. |
| src/blob/generated-dfs/handlers/IFilesystemHandler.ts | Generated DFS filesystem handler interface. |
| src/blob/generated-dfs/artifacts/specifications.ts | Generated DFS dispatch specifications. |
| src/blob/generated-dfs/artifacts/operation.ts | Generated DFS operation enum. |
| src/blob/generated-dfs/artifacts/models.ts | Generated DFS request/response models. |
| src/blob/generated-dfs/Context.ts | DFS context wrapper for generated handler pattern. |
| src/blob/dfs/handlers/PathHandler.ts | Implements DFS path operations (create/delete/read/list/update/rename/lease/ACL). |
| src/blob/dfs/handlers/FilesystemHandler.ts | Implements DFS filesystem operations mapped to containers. |
| src/blob/dfs/DfsPropertyEncoding.ts | Adds encoding/decoding helper for x-ms-properties. |
| src/blob/dfs/DfsOperation.ts | Defines DFS operation identifiers for dispatch/auth. |
| src/blob/dfs/DfsErrorFactory.ts | DFS JSON error factory + HEAD-safe error responses. |
| src/blob/dfs/DfsContextFactory.ts | Creates minimal blob Context objects for DFS handlers to use stores. |
| src/blob/dfs/DfsContext.ts | DFS context extraction middleware + identity container. |
| src/blob/dfs/DfsAuthenticationMiddleware.ts | DFS authentication middleware reusing blob authenticators + identity extraction. |
| src/blob/dfs/DfsAclEnforcer.ts | Implements ACL parsing and permission evaluation for DFS paths. |
| src/blob/context/BlobStorageContext.ts | Adds environment field and minor formatting fixes. |
| src/blob/SqlBlobServer.ts | Exposes internal stores publicly (for DFS sharing/tests). |
| src/blob/IBlobEnvironment.ts | Extends env interface for DFS host/port and HNS enablement. |
| src/blob/DfsServer.ts | Adds standalone DFS server type sharing blob stores. |
| src/blob/DfsRequestListenerFactory.ts | Implements DFS Express router and pipeline (dispatch/auth/HNS validation). |
| src/blob/DfsConfiguration.ts | Adds DFS configuration class. |
| src/blob/BlobServerFactory.ts | Wires HNS enablement into BlobConfiguration creation. |
| src/blob/BlobServer.ts | Passes HNS flag into request listener factory and exposes stores publicly. |
| src/blob/BlobRequestListenerFactory.ts | Mounts DFS router into blob listener and threads HNS flag to handlers. |
| src/blob/BlobEnvironment.ts | Adds CLI flags for DFS host/port and HNS enablement. |
| src/blob/BlobConfiguration.ts | Adds enableHierarchicalNamespace configuration field. |
| src/azurite.ts | Updates console output and adds DFS deprecation warning messaging. |
| package.json | Adds DFS SDK dependency, VS Code settings, and an AutoRest DFS build script. |
| docs/designs/ADLS-gen2-parity.md | Adds design doc describing phased ADLS Gen2 parity implementation. |
| README.md | Documents DFS host/port settings and Docker port exposure. |
| Dockerfile.Windows | Exposes DFS port and adds dfsHost to container command. |
| Dockerfile | Exposes DFS port and adds dfsHost to container command. |
Comments suppressed due to low confidence (1)
src/blob/dfs/handlers/PathHandler.ts:1163
safeGetBlobPropertiescallsgetBlobPropertieswith a context created viacreateStorageContext()(no requestId). This loses request correlation and can be problematic if the underlying store expectscontext.contextIdto be populated. Consider accepting a requestId (or Context) parameter and passingcreateStorageContext(ctx.requestId)here as well.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Addressed the remaining minor findings from Copilot review body (suppressed comments, no inline threads existed for these) — fixed in 709ba9f:
Not acted on (judged style/architecture nitpicks, not real bugs — left open):
|
There was a problem hiding this comment.
🟡 Changes recommended
ACL mode can be bypassed by bearer tokens lacking oid/upn, and the VS Code HNS setting appears not to be wired into server configuration.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/common/VSCEnvironment.ts:142
enableHierarchicalNamespace()is added here, but there are no call sites using it (only BlobEnvironment/Environment are wired). As a result, the VS Code settingazurite.enableHierarchicalNamespacewon’t affect the Blob/DFS server configuration when started via the extension (BlobConfiguration is constructed elsewhere without passing this value).
- Files reviewed: 54/57 changed files
- Comments generated: 1
- Review effort level: Lite
… into BlobConfiguration (VSCEnvironment.ts:142) VSCServerManagerBlob's getConfiguration() built BlobConfiguration without passing env.enableHierarchicalNamespace(), so the azurite.enableHierarchicalNamespace VS Code setting had no effect on the Blob/DFS server when started via the extension.
|
Fixed by f28e674 — wired |
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces a large new API surface (DFS), routing/auth/persistence changes, and substantial new behavior that warrants final human validation beyond targeted spot-checks.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/blob/dfs/handlers/FilesystemHandler.ts:142
maxResultsparsing usesparseInt(...) || 5000, which treats an explicitmaxResults=0as falsy and silently replaces it with 5000. That breaks the intended clamp-to-[1..5000] behavior and can cause unexpectedly large listings for invalid/edge inputs. Parse first, then default only on NaN.
- Files reviewed: 55/58 changed files
- Comments generated: 0 new
- Review effort level: Lite
…review 5169525198, FilesystemHandler.ts:142) parseInt(...) || 5000 treated an explicit maxResults=0 as falsy and silently replaced it with 5000, breaking the intended clamp-to-[1..5000] behavior for that edge case. Parse first, then only default to 5000 on NaN (unspecified/invalid input), then clamp.
|
Addressed remaining Copilot suppressed-comment finding from review 5169525198:
Fixed by 3d3d25b. All other outstanding suppressed-comment findings from prior reviews (SQL LIKE escaping, listPaths dedup, DFS URL parsing, position validation, lease/action routing, ChangeLog -hns alias, OAuthLevel.ACL handling, VS Code HNS wiring, minor test/doc nits) were already fixed in earlier commits on this branch and Copilot is re-listing them from stale snapshots since the code hasn't changed since. No new inline review threads or merge conflicts (mergeable=true, mergeStateStatus=null) as of head f28e674 → 3d3d25b. |
There was a problem hiding this comment.
🔵 Needs a closer look
ACL mode currently creates DFS paths (and intermediate directories) without default dfsAcl metadata for the caller, and a new ACL unit test asserts semantics that contradict the implemented enforcement behavior.
Review details
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
src/blob/dfs/handlers/PathHandler.ts:65
- In
--oauth aclmode, paths created via Bearer auth currently do not get any initial ACL metadata (dfsAclOwner/dfsAclPermissions/etc.). BecauseenforceAcl()consults these metadata fields and defaults to owner="$superuser" with permissions "rwxr-x---", the creator will typically be unable to append/flush/read/delete immediately after creating the path (403), which diverges from Azure behavior where the creator becomes the owner. Consider setting default ACL metadata for newly created files/directories whenctx.identityis present.
This issue also appears on line 1212 of the same file.
tests/blob/dfsAclEnforcer.test.ts:74
checkAcl()does not (and should not) bypass ACL enforcement just because the stored owner is "$superuser" (or undefined). These two tests currently assert bypass semantics that would contradict the DFS ACL enforcement integration test cases (e.g. owner="$superuser" with a named-user ACL entry). Update the expectations here to match the implemented evaluation (owner/group/default permissions) rather than treating "$superuser" ownership as an automatic allow.
src/blob/dfs/handlers/PathHandler.ts:1216
ensureIntermediateDirectories()creates intermediate directory blobs with onlyhdi_isfolder=truemetadata. In--oauth aclmode, these intermediate directories will then exist without any dfsAcl* metadata, so subsequent Bearer-authenticated DFS operations that touch them (e.g. creating siblings, listing, or renaming under that prefix) are likely to be denied byenforceAcl(). The intermediate directories should probably be stamped with the same default owner/permissions as the leaf path (or inherit from the parent), which will require threading identity/owner information into this helper and its call sites.
if (!existing) {
const dirBlob: BlobModel = {
accountName: account,
containerName: filesystem,
name: dirPath,
- Files reviewed: 55/58 changed files
- Comments generated: 0 new
- Review effort level: Lite
… (Copilot review, DfsAclEnforcer.ts:131) Since BlobTokenAuthenticator.authenticateBasic accepts Bearer tokens without signature validation, a caller could previously omit both oid and upn claims and be treated as 'emulator mode', bypassing ACL enforcement entirely for authenticated requests. Now only a completely absent identity (no Bearer token presented) bypasses; an identity object missing both claims is denied (fail closed) since we cannot evaluate ACLs without an identifiable caller. Updated dfsAclEnforcer.test.ts expectations that had asserted the old bypass-on-missing-claims and bypass-on-$superuser-as-stored-owner behavior, which contradicted the actual enforceAcl() semantics (only an actual $superuser *caller* bypasses, not a $superuser-valued owner/group field).
|
Automated pass — fixed one confirmed ACL bypass, evaluated the rest: Fixed (658c6c2): Confirmed already fixed by prior commits in this PR: Real but deferred (needs a design decision, not a mechanical fix): No merge conflicts (mergeable=true, mergeStateStatus=null against main). |
…review) Found by an independent Claude-driven review of PR Azure#2635, covering issues GitHub Copilot's automated review passes never caught. All 6 issues below are classified Critical (auth bypass or write-path integrity). 1. Unauthenticated DFS access bypass (DfsAuthenticationMiddleware.ts): removed the blanket "no credentials + no --oauth => pass through" branch, which let ANY request through with zero credentials whenever the server wasn't started with --oauth. This was remotely triggerable against the Blob endpoint too, since BlobRequestListenerFactory routes any request whose User-Agent contains "datalake" (fully client-controlled) into the DFS pipeline. Added PublicAccessAuthenticator to the DFS authenticator chain (same as the Blob pipeline already has) so anonymous access is only ever granted via a real container public-access check, never a blanket pass-through. Documented that the UA-based routing heuristic must never be treated as an authorization signal, now that both pipelines independently enforce real auth. 2. lease() had zero ACL enforcement (PathHandler.ts): any authenticated caller in --oauth acl mode could acquire/break/steal a lease on any path regardless of ACL. Added enforceAcl(..., "w") at the top of lease() before dispatch to the sub-actions. 3. Append/flush bypassed lease enforcement entirely (PathHandler.ts): stageBlock() was called with leaseAccessConditions hardcoded to undefined, and commitBlockList() wasn't passed any conditions at all, so a client with no lease could append/flush data to a leased file. Now both extract and thread x-ms-lease-id through to the metadata store, which validates it via the same BlobWriteLeaseValidator/ BlobReadLeaseValidator used by the Blob pipeline. Also added 412 ConditionNotMet mapping to both handlers' catch blocks, and closed a gap where the "nothing to flush" fast path skipped the position check entirely (a flush(position=<anything>) on an unmodified file returned a false 200 instead of validating against the actual committed length). 4. Rename force-deleted the destination with zero lease/conditional-header checks (PathHandler.ts renamePath): the destination could be under an active lease and would still be silently overwritten. Now extracts x-ms-lease-id/If-Match/etc. for the destination and threads them into deleteBlob(); a leased destination without the correct lease id now returns 412 instead of being deleted. Also added x-ms-source-lease-id support: the source blob is now fetched via getBlobProperties() with lease conditions (instead of the ACL-only safeGetBlobProperties), so a leased source without the matching source-lease-id is rejected before any mutation happens. 5. create() ignored conditional headers entirely (PathHandler.ts): there was no If-None-Match/overwrite=false handling for files (only directories had an existence check), so Azurite always overwrote silently where real ADLS Gen2 returns 409 PathAlreadyExists. Now extracts modifiedAccessConditions/leaseAccessConditions and passes them into createBlob(), which already supported and validated them. 6. Rename didn't validate type compatibility (PathHandler.ts renamePath): a file could silently replace an empty directory, or a directory could replace a file, since only "non-empty directory" was rejected. Added an explicit isDir-vs-destIsDir check that returns 409 PathConflict before any destination deletion happens. Fixed 2 pre-existing tests (dfsReproduction.test.ts) that only passed because they relied on the removed anonymous-access bypass; switched them to use account SAS like every other DFS test file. Added 7 new regression tests across dfsProxy.test.ts and dfsAclIntegration.test.ts covering each of the 6 fixes above (lease-gated append/rename, rename type-mismatch in both directions, destination-lease-respecting rename, lease-gated ACL enforcement, and overwrite=false create semantics). Verified: tsc --noEmit clean, eslint clean, DFS suite 67/67 passing (was 61 + 6 new), full @loki suite 639/639 passing (was 633 + 6 new).
Independent code review (Claude), commit 9a7dc49Since automated Copilot reviews hit their quota, I ran a from-scratch manual review of the DFS routing/auth/handler/persistence layers. Found and fixed 6 Critical-severity issues that hadn't previously surfaced:
Added 7 new regression tests covering each fix ( Verified: Full details in the commit message of 9a7dc49. |
Continuing the independent Claude review of PR Azure#2635. Fixes the following warnings (numbering matches the review summary posted earlier): 1. HNS default `false` in 4 handler constructors (ContainerHandler, ServiceHandler, BlobHandler, BlobBatchHandler) contradicted the server-wide default of `true` used everywhere else. Currently dead code in the shipped server paths (BlobServer/SqlBlobServer always pass the flag explicitly), but a landmine for any future/alternate construction path (tests, embedding) that forgets to thread it. All 4 now default to EMULATOR_ACCOUNT_ISHIERARCHICALNAMESPACEENABLED_DEFAULT. 2. ServiceHandler.listContainersSegment leaked the internal azurite_hns_enabled metadata key to clients with include=metadata, unlike GetContainerProperties and DFS getProperties which both filter it. Applied the same filter. 3. DfsContext's URL/path parsing (extractDfsPartsFromPath) could throw a raw URIError on a malformed percent-encoded segment, which Express's default error handler would turn into an HTML/plain response instead of the documented DFS JSON error contract. Wrapped in try/catch, returns 400 InvalidUri. 4. DfsAuthenticationMiddleware's unclassified-operation fallback mapped to a read op (Blob_GetProperties) for SAS permission checking — the least restrictive choice. Changed to the most restrictive (BlockBlob_Upload) so an as-yet-unclassified request is never under-enforced. 5. DfsRequestListenerFactory's terminal error handler flattened everything to a generic 500 and echoed error.message (possible internal detail leak) verbatim to the client. Now maps known body-parser error shapes (entity.too.large -> 413, entity.parse.failed -> 400) and returns a sanitized message for the true 500 fallback, logging the real error server-side instead. 6. DfsAclEnforcer silently fell through named-group ACL entries (no AAD membership resolution in the emulator) to "other" permissions with no signal to the operator. Added a one-time warn-level log per check when named-group entries are present but unevaluated. 8. setAccessControlRecursive only ACL-checked the root path (via update()'s top-level enforceAcl), not each descendant — a caller with write only on a directory root could rewrite ACLs arbitrarily deep into a subtree. Added a per-child check via a new checkAclSilent() helper (denied children are now counted in failureCount instead of aborting the whole operation, consistent with existing per-item error handling). 9. PathHandler.read() ignored Range/x-ms-range entirely, always streaming the full blob and never returning 206/Content-Range. Ported the same range-parsing/clamping logic BlobHandler.downloadBlockBlobOrAppendBlob already uses (deserializeRangeHeader + IExtentStore.readExtents), so DFS reads now support partial/resumable downloads like the Blob API. 10. Rename's destructive destination-delete happened before intermediate directory creation, unnecessarily widening the failure window where a crash could leave the destination path missing entirely. Reordered so the delete is the last step before the atomic rename when a destination exists (and skipped entirely — nothing destructive to do — when it doesn't, in which case only intermediate dirs are created). 11. setAccessControl let any caller with mere write access to a path reassign x-ms-owner, letting them grant themselves owner rights on future requests. Real ADLS Gen2 restricts ownership changes to the current owner or $superuser; added that check (only enforced when ACL mode + identity are active, matching how the rest of enforceAcl works). Added 4 new regression tests: Range/206 support (multiple ranges + beyond- EOF 416), and owner-reassignment denial/allowance in dfsAclIntegration. Not yet addressed (remaining Warnings, deferred as lower severity / larger scope): Azure#7 (recursive delete concurrency-limited but ACL-recursive paging not unified with it — already correctly pages via continuation, noted as a documentation nit not a bug), Azure#12 (TOCTOU between enforceAcl and the subsequent mutating call — accepted emulator limitation, same class as the already-documented append race), Azure#13/Azure#14 (named-group ACL / permission-model documentation nits), and SQL-portability Suggestions from the earlier persistence-layer review pass. Verified: tsc --noEmit clean, eslint clean, DFS suite 69/69 passing (was 67 + 2 new), full @loki suite 641/641 passing (was 639 + 2 new).
Independent code review (Claude), commit 47a3070 — 9/14 Warnings addressedContinuing the manual review from the previous comment. Fixed 9 of the 14 Warning-severity findings:
Added 4 new regression tests (Range/206 + 416 beyond-EOF, owner-reassignment denial/allowance). Deferred (lower severity / larger scope, not yet fixed): #7 (ACL-recursive paging documentation nit — already correct), #12 (TOCTOU between ACL check and mutation — same accepted-limitation class as the documented append race), #13/#14 (documentation-only nits), and the SQL-portability Suggestions from the persistence-layer pass. Verified: |
Closes out the review findings not addressed in the previous two commits. Documentation/doc-only fixes: - Azure#12: documented the TOCTOU window between enforceAcl() and the mutating operation that follows it as an accepted emulator limitation, same class as the already-documented append-position race. - Azure#13: (already covered by the earlier Azure#6 fix — named-group ACL entries now log a warning when present-but-unevaluated.) - Azure#14: documented in getRequiredPermission() that "setAccessControl" shares the same "w" permission bucket as ordinary writes, and that PathHandler.setAccessControl separately guards x-ms-owner reassignment specifically (the one case with real privilege-escalation risk) rather than the permission model distinguishing "write data" from "write ACL" in general. - Azure#7: reviewed setAccessControlRecursive/delete's pagination — both already correctly page via continuation/marker; no action needed (the original finding was a documentation nit, not a bug). SQL portability fix (Suggestion): - SqlBlobMetadataStore's escapeLike()/likePrefixCondition()/ conditionalPrefixReplaceExpr() used '\' as the LIKE escape character, requiring MySQL/MariaDB-specific doubled-backslash handling that silently breaks if the target server runs with the NO_BACKSLASH_ESCAPES sql_mode (backslash stops being a string-literal metacharacter, so the doubled-escape clause becomes wrong). Switched the escape character to '^', which has no special meaning in any supported dialect's string literal syntax (sqlite/postgres/mysql/mariadb/mssql), eliminating the sql_mode dependency entirely and simplifying the code (no more per-dialect ESCAPE clause branching). Added a regression test renaming a directory whose name contains literal '%'/'_' characters, verifying an unrelated sibling blob that would spuriously match if those characters were misinterpreted as wildcards is left untouched. Verified: tsc --noEmit clean, eslint clean, DFS suite 70/70 passing (was 69 + 1 new), full @loki suite 642/642 passing (was 641 + 1 new). Note: the SQL escape-character change could not be verified against a live MySQL/MariaDB/MSSQL server in this environment (no such server available here); it was validated for syntactic consistency with the existing per-dialect quoting in the same file and the @loki test suite, but a maintainer with SQL-backend CI access should confirm the @SQL suite still passes before merge.
Independent code review (Claude), commit ac65cd3 — all remaining findings addressedClosing out the review from the previous two comments. This commit resolves the remaining items:
Added a regression test renaming a directory with literal Note on verification: I don't have access to a live MySQL/MariaDB/MSSQL server in this environment, so the SQL escape-character change is verified for syntactic correctness and consistency with the existing per-dialect quoting in the file, and passes the full Summary across all three review passes: all 6 Critical findings fixed, all 14 Warnings/Suggestions addressed (9 code fixes + 5 documentation/already-correct), 8 new regression tests added. Test suite: 633 → 642 passing, all green. |
Adds ADLS Gen 2 feature parity to Azurite. See docs/design/ADLS-gen2-parity.md file for details.
This was created by Claude Sonnet 4.6. Just sharing my progress here.
See OrchardCMS/OrchardCore#19014 for some integration tests.
The Github CI over there uses the current latest Docker image but I've succesfully ran my unit tests using a local Docker container based on this PR.
Maybe we need to create more unit tests. I let you guys decide, but it is looking good for a start.