feat: add rust fs and local storage support#1670
Open
sujitaw wants to merge 6 commits into
Open
Conversation
Signed-off-by: sujitaw <sujit.sutar@ayanworks.com>
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (36)
💤 Files with no reviewable changes (4)
✅ Files skipped from review due to trivial changes (7)
📝 WalkthroughWalkthroughThis PR replaces AWS-specific storage implementation with a pluggable abstraction supporting multiple backends (AWS S3, RustFS, local filesystem). A new StorageService façade selects the concrete provider at runtime based on FILE_STORAGE_TYPE environment variable. All application modules are updated to inject StorageService instead of AwsService, and environment variables are renamed for consistency across backends. ChangesMulti-Backend Storage Abstraction Migration
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Suggested Reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
Author
|
@coderabbitai fullreview |
|
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.env.demo:
- Around line 137-139: Replace the concrete credentials in the .env template by
removing the weak defaults and using empty placeholders for RUSTFS_ACCESS_KEY_ID
and RUSTFS_SECRET_ACCESS_KEY (e.g. set them to empty values) and keep
RUSTFS_REGION as a default region if desired; also add a short inline comment in
the file near RUSTFS_ACCESS_KEY_ID / RUSTFS_SECRET_ACCESS_KEY that clearly
states these must be supplied at deploy time and must not be checked in. This
ensures the environment variables RUSTFS_ACCESS_KEY_ID and
RUSTFS_SECRET_ACCESS_KEY are not hard-coded and documents the requirement to
provide real secrets during deployment.
In `@apps/api-gateway/src/main.ts`:
- Line 125: The unconditional mount app.use('/uploadedFiles',
express.static('uploadedFiles')) exposes the whole storage tree; change this to
only mount explicit safe subdirectories (e.g., '/uploadedFiles/public' ->
express.static(path.join('uploadedFiles','public'))) and/or wrap the mount in a
feature-flag check (e.g., isLocalStorageEnabled() or
process.env.LOCAL_STORAGE_ENABLED) so the static middleware is registered only
when local storage serving is allowed; ensure you reference and update the
app.use call and the '/uploadedFiles' mount point accordingly and prefer
path.join for safe paths and a whitelist of allowed subfolders.
In `@libs/aws/src/aws.service.ts`:
- Around line 20-26: The constructor returns early when this.isLocalFs is true
but later code still expects AWS_BUCKET, AWS_ORG_LOGO_BUCKET_NAME, and
AWS_S3_STOREOBJECT_BUCKET to be set, causing path.join with undefined; update
the constructor (around the this.isLocalFs / localStoragePath logic) to validate
those env vars when this.isLocalFs is true and fail fast by throwing a clear
error (or set safe defaults) before returning so subsequent local file path
operations (that use those bucket envs) won’t receive undefined.
- Around line 155-158: The local delete in aws.service.ts currently does await
fs.unlink(filePath) inside the isLocalFs branch which will throw if the file
doesn't exist; make this idempotent like S3 by wrapping the unlink in a
try/catch inside the same method (the isLocalFs branch) and swallow errors whose
code === 'ENOENT' but rethrow any other errors so only missing-file cases are
ignored; ensure the try/catch surrounds the call referencing the same
localStoragePath/process.env.AWS_BUCKET/key resolution used to build filePath.
- Around line 81-87: Validate and normalize any user-supplied path segments
(pathAWS, key, objKey, filename) before joining them into filesystem paths:
strip or reject path traversal like "../" (e.g., replace with "" or use
path.basename for filename segments), use path.normalize on the combined path,
then compute the final filePath by path.join(bucketDir, normalizedSegment) and
verify the resulting absolute path starts with the bucketDir absolute path
(using path.resolve) — if it does not, throw an error. Apply this same
validation/sanitization logic wherever pathAWS/key/objKey are used (references:
bucketDir, fileKey, filePath, ensureDir, localStoragePath) to prevent escaping
uploadedFiles/<bucket> in local mode. Ensure returned URL still uses the
original safe URL-encoded filename but only after the filesystem checks pass.
- Around line 116-120: uploadCsvFile currently only ensures the bucket root dir
(using ensureDir on bucketDir) but then writes filePath directly, which fails
for nested keys like "foo/bar.csv"; update uploadCsvFile to create parent
directories for the final file by calling ensureDir on the file's directory
(e.g. path.dirname(filePath)) before calling fs.writeFile so nested prefixes in
key behave like S3; reference the existing localStoragePath,
process.env.AWS_BUCKET, key, filePath, ensureDir, and fs.writeFile when making
this change.
- Around line 49-64: The S3 clients (this.s3, this.s4, this.s3StoreObject)
always read AWS_*_REGION env vars but when IS_LOCAL_RUSTFS is set you should
prefer RUSTFS_REGION; update the region selection logic in the constructor
(where these S3 instances are created) to use process.env.RUSTFS_REGION when
process.env.IS_LOCAL_RUSTFS is truthy, falling back to the existing AWS_*_REGION
values otherwise so RustFS mode uses the correct region.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b0c19a79-f348-4b2d-9ee3-990730efae19
📒 Files selected for processing (5)
.env.demoapps/api-gateway/src/main.tsapps/issuance/src/issuance.service.tsapps/utility/src/utilities.service.tslibs/aws/src/aws.service.ts
💤 Files with no reviewable changes (1)
- apps/issuance/src/issuance.service.ts
Signed-off-by: sujitaw <sujit.sutar@ayanworks.com>
Signed-off-by: sujitaw <sujit.sutar@ayanworks.com>
Signed-off-by: sujitaw <sujit.sutar@ayanworks.com>
|
sujitaw
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
188-209:⚠️ Potential issue | 🟠 MajorAdd missing Jest
moduleNameMapperentry for@credebl/storage
tsconfig.jsondefines the@credebl/storagepath aliases, butpackage.json’s JestmoduleNameMapperlacks^@credebl/storage(|/.*)$. Multiple apps importStorageServicefrom@credebl/storage, so Jest can fail with unresolved module errors.Suggested fix
"moduleNameMapper": { + "^`@credebl/storage`(|/.*)$": "<rootDir>/libs/storage/src/$1", "`@credebl/common/`(.*)": "<rootDir>/libs/common/src/$1", "`@credebl/common`": "<rootDir>/libs/common/src", "`@credebl/keycloak-url/`(.*)": "<rootDir>/libs/keycloak-url/src/$1", "`@credebl/keycloak-url`": "<rootDir>/libs/keycloak-url/src", "`@credebl/client-registration/`(.*)": "<rootDir>/libs/client-registration/src/$1", "`@credebl/client-registration`": "<rootDir>/libs/client-registration/src", "^`@credebl/prisma`(|/.*)$": "<rootDir>/libs/prisma/src/$1", "^`@credebl/repositories`(|/.*)$": "<rootDir>/libs/repositories/src/$1", "^`@credebl/user-request`(|/.*)$": "<rootDir>/libs/user-request/src/$1", "^`@credebl/enum`(|/.*)$": "<rootDir>/libs/enum/src/$1", "^`@credebl/prisma-service`(|/.*)$": "<rootDir>/libs/prisma-service/src/$1", "^`@credebl/org-roles`(|/.*)$": "<rootDir>/libs/org-roles/src/$1", "^`@credebl/user-org-roles`(|/.*)$": "<rootDir>/libs/user-org-roles/src/$1", "^y/user-activity(|/.*)$": "<rootDir>/libs/user-activity/src/$1", "^`@app/supabase`(|/.*)$": "<rootDir>/libs/supabase/src/$1", "^credebl/utility(|/.*)$": "<rootDir>/libs/utility/src/$1", "^`@credebl/config`(|/.*)$": "<rootDir>/libs/config/src/$1", "^`@credebl/context`(|/.*)$": "<rootDir>/libs/context/src/$1", "^`@credebl/logger`(|/.*)$": "<rootDir>/libs/logger/src/$1" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package.json` around lines 188 - 209, The Jest moduleNameMapper in package.json is missing the `@credebl/storage` alias, causing tests to fail to resolve imports like StorageService; add an entry to the "moduleNameMapper" object mapping the regex ^`@credebl/storage`(|/.*)$ to the corresponding source path (same style as other entries) so Jest can resolve imports from `@credebl/storage`; update the "moduleNameMapper" section (where other patterns like ^`@credebl/prisma`(|/.*)$ and ^`@credebl/logger`(|/.*)$ appear) to include the storage mapping.
🧹 Nitpick comments (3)
apps/api-gateway/src/issuance/issuance.module.ts (1)
28-28: ImportingStorageModuleisn’t required for DI correctness here; it’s optional for consistency
StorageServicehas a parameterless constructor and instantiatesLocalFsStorageService/RustFsStorageService/S3StorageServicewithnew(env-based), and those provider constructors also take no injected dependencies. This means providingStorageServicedirectly inapps/api-gateway/src/issuance/issuance.module.tsshouldn’t cause Nest DI resolution failures. ImportingStorageModulewould only be a consistency/future-proofing improvement.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api-gateway/src/issuance/issuance.module.ts` at line 28, The module currently lists StorageService directly in providers while also importing StorageModule unnecessarily; since StorageService and its concrete implementations (LocalFsStorageService, RustFsStorageService, S3StorageService) have parameterless constructors and are instantiated with new, remove the unnecessary import of StorageModule from the IssuanceModule (or alternatively explicitly import StorageModule for consistency/future-proofing) so DI remains correct; locate the providers array containing IssuanceService, CommonService, StorageService, NATSClient and either drop the StorageModule import or add the StorageModule to the module's imports for consistency.libs/storage/src/storage.service.ts (1)
12-23: ⚡ Quick winProvider selection currently bypasses Nest DI by constructing classes manually.
Using
new LocalFsStorageService()/RustFsStorageService()/S3StorageService()bypasses lifecycle/scoping and duplicates module wiring. Prefer constructor injection of providers, then select the injected instance byFILE_STORAGE_TYPE.Refactor sketch
- private storage: IStorageService; - - constructor() { + private readonly storage: IStorageService; + constructor( + private readonly localFsStorageService: LocalFsStorageService, + private readonly rustFsStorageService: RustFsStorageService, + private readonly s3StorageService: S3StorageService + ) { const storageType = process.env.FILE_STORAGE_TYPE || 'aws'; switch (storageType) { case 'local': - this.storage = new LocalFsStorageService(); + this.storage = this.localFsStorageService; break; case 'rustfs': - this.storage = new RustFsStorageService(); + this.storage = this.rustFsStorageService; break; default: - this.storage = new S3StorageService(); + this.storage = this.s3StorageService; break; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/storage/src/storage.service.ts` around lines 12 - 23, The constructor is instantiating providers directly (new LocalFsStorageService(), new RustFsStorageService(), new S3StorageService()), bypassing Nest's DI and lifecycle; change StorageService to accept the three provider instances via constructor injection (e.g., add parameters for LocalFsStorageService, RustFsStorageService, S3StorageService or use `@Inject` tokens) and in the constructor select which injected instance to assign to this.storage based on process.env.FILE_STORAGE_TYPE (or FILE_STORAGE_TYPE constant) instead of using new; keep the switch that assigns this.storage but use the injected symbols LocalFsStorageService, RustFsStorageService, S3StorageService so Nest can manage scopes and lifecycle.libs/storage/src/providers/local-fs-storage.provider.ts (1)
55-59: Correct the “undeclared AWS namespace” claim; this is mainly a consistency issue
AWS.S3.*types resolve becauseaws-sdktypings declare a globalAWSnamespace (export as namespace AWSinnode_modules/aws-sdk/index.d.ts). The real concern is consistency: this module mixesS3imports withAWS.S3.*return/param types—consider switchingPromise<S3.GetObjectOutput>/as S3.GetObjectOutputfor consistency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/storage/src/providers/local-fs-storage.provider.ts` around lines 55 - 59, The code mixes global AWS namespace types with the imported S3 type; update getFile to use the S3 type for consistency by changing the return type from Promise<AWS.S3.GetObjectOutput> to Promise<S3.GetObjectOutput> and cast the returned object as S3.GetObjectOutput (same for any other methods in this file using AWS.S3.*). Locate the getFile function in local-fs-storage.provider.ts and replace AWS.S3.* references with the imported S3.* type so all S3 types are consistent across the module.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/storage/src/providers/base-s3-storage.provider.ts`:
- Around line 31-39: Normalize the optional pathAWS before building object keys:
trim any leading/trailing slashes from pathAWS (or set it to '' if empty) and
only include the `${pathAWS}/` prefix in fileKey and the putObjectAsync Key when
pathAWS is non-empty; update the construction of fileKey and the Key passed to
putObjectAsync (which currently mirror each other) to use the normalized pathAWS
so keys become `filename-<ts>.ext` when no path is provided and
`path/filename-<ts>.ext` otherwise, keeping the existing
encodeURIComponent(filename), timestamp, and extension logic and continuing to
use putObjectAsync and this.s4.putObject.
- Around line 50-55: The uploadCsvFile method currently coerces any body via
body.toString(), which can turn plain objects into "[object Object]" and corrupt
CSVs; update uploadCsvFile to validate that the body is either a string or a
Buffer before creating the PutObjectRequest (reject or throw for other types),
and ensure the PutObjectRequest.Body uses the original string or Buffer value;
reference the uploadCsvFile function and the PutObjectRequest construction to
locate where to add the type check and error handling.
In `@libs/storage/src/providers/local-fs-storage.provider.ts`:
- Around line 47-53: The functions uploadCsvFile, getFile, deleteFile, and
storeObject are vulnerable to path traversal because they join user-supplied
keys into paths directly; update each to resolve the target path using the
existing resolveUnderBase helper (or equivalent) so the final file path is
guaranteed under the bucket directory before any fs ops. Specifically: compute
bucketDir as now, then call resolveUnderBase(bucketDir, key) (and for
storeObject also for objFilePath) to get the safe filePath, use path.dirname on
that safe path for ensureDir, and perform write/read/unlink on the resolved path
instead of path.join(bucketDir, key). Ensure this pattern is applied
consistently in uploadCsvFile, getFile, deleteFile, and storeObject.
- Around line 36-43: The bug is that when pathAWS is the empty string the
template `${pathAWS}/${encodeURIComponent(filename)}-...` produces a leading
slash, so resolveUnderBase(bucketDir, fileKey) escapes bucketDir and throws; fix
by normalizing/building the key without introducing a leading slash: trim any
leading/trailing slashes from pathAWS and only prepend it plus a slash when
non-empty (or use path.posix.join to join pathAWS and the filename component),
ensure the same normalized pathAWS (or joined path) is used for the ensureDir
call and for fileKey, and keep resolveUnderBase(bucketDir, fileKey) unchanged so
it will resolve inside bucketDir.
---
Outside diff comments:
In `@package.json`:
- Around line 188-209: The Jest moduleNameMapper in package.json is missing the
`@credebl/storage` alias, causing tests to fail to resolve imports like
StorageService; add an entry to the "moduleNameMapper" object mapping the regex
^`@credebl/storage`(|/.*)$ to the corresponding source path (same style as other
entries) so Jest can resolve imports from `@credebl/storage`; update the
"moduleNameMapper" section (where other patterns like ^`@credebl/prisma`(|/.*)$
and ^`@credebl/logger`(|/.*)$ appear) to include the storage mapping.
---
Nitpick comments:
In `@apps/api-gateway/src/issuance/issuance.module.ts`:
- Line 28: The module currently lists StorageService directly in providers while
also importing StorageModule unnecessarily; since StorageService and its
concrete implementations (LocalFsStorageService, RustFsStorageService,
S3StorageService) have parameterless constructors and are instantiated with new,
remove the unnecessary import of StorageModule from the IssuanceModule (or
alternatively explicitly import StorageModule for consistency/future-proofing)
so DI remains correct; locate the providers array containing IssuanceService,
CommonService, StorageService, NATSClient and either drop the StorageModule
import or add the StorageModule to the module's imports for consistency.
In `@libs/storage/src/providers/local-fs-storage.provider.ts`:
- Around line 55-59: The code mixes global AWS namespace types with the imported
S3 type; update getFile to use the S3 type for consistency by changing the
return type from Promise<AWS.S3.GetObjectOutput> to Promise<S3.GetObjectOutput>
and cast the returned object as S3.GetObjectOutput (same for any other methods
in this file using AWS.S3.*). Locate the getFile function in
local-fs-storage.provider.ts and replace AWS.S3.* references with the imported
S3.* type so all S3 types are consistent across the module.
In `@libs/storage/src/storage.service.ts`:
- Around line 12-23: The constructor is instantiating providers directly (new
LocalFsStorageService(), new RustFsStorageService(), new S3StorageService()),
bypassing Nest's DI and lifecycle; change StorageService to accept the three
provider instances via constructor injection (e.g., add parameters for
LocalFsStorageService, RustFsStorageService, S3StorageService or use `@Inject`
tokens) and in the constructor select which injected instance to assign to
this.storage based on process.env.FILE_STORAGE_TYPE (or FILE_STORAGE_TYPE
constant) instead of using new; keep the switch that assigns this.storage but
use the injected symbols LocalFsStorageService, RustFsStorageService,
S3StorageService so Nest can manage scopes and lifecycle.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 108666eb-25c5-4d82-a24e-dc7c994db6c7
📒 Files selected for processing (36)
.env.demo.gitignoreapps/api-gateway/src/issuance/issuance.controller.tsapps/api-gateway/src/issuance/issuance.module.tsapps/api-gateway/src/organization/organization.module.tsapps/api-gateway/src/user/user.controller.tsapps/api-gateway/src/user/user.module.tsapps/api-gateway/src/webhook/webhook.module.tsapps/api-gateway/src/x509/x509.module.tsapps/issuance/src/issuance.module.tsapps/issuance/src/issuance.service.tsapps/organization/src/organization.module.tsapps/organization/src/organization.service.tsapps/user/src/fido/fido.module.tsapps/user/src/user.module.tsapps/user/src/user.service.tsapps/utility/src/utilities.module.tsapps/utility/src/utilities.service.tslibs/aws/src/aws.module.tslibs/aws/src/aws.service.spec.tslibs/aws/src/index.tslibs/storage/package.jsonlibs/storage/src/index.tslibs/storage/src/providers/base-s3-storage.provider.tslibs/storage/src/providers/index.tslibs/storage/src/providers/local-fs-storage.provider.tslibs/storage/src/providers/rustfs-storage.provider.tslibs/storage/src/providers/s3-storage.provider.tslibs/storage/src/storage.interface.tslibs/storage/src/storage.module.tslibs/storage/src/storage.service.tslibs/storage/tsconfig.build.jsonlibs/storage/tsconfig.jsonnest-cli.jsonpackage.jsontsconfig.json
💤 Files with no reviewable changes (4)
- libs/aws/src/aws.service.spec.ts
- libs/aws/src/aws.module.ts
- libs/aws/src/index.ts
- nest-cli.json
✅ Files skipped from review due to trivial changes (7)
- libs/storage/tsconfig.build.json
- libs/storage/src/index.ts
- libs/storage/src/providers/index.ts
- libs/storage/package.json
- libs/storage/src/storage.interface.ts
- apps/api-gateway/src/issuance/issuance.controller.ts
- apps/organization/src/organization.service.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
188-209:⚠️ Potential issue | 🟠 MajorAdd missing Jest
moduleNameMapperentry for@credebl/storage
tsconfig.jsondefines the@credebl/storagepath aliases, butpackage.json’s JestmoduleNameMapperlacks^@credebl/storage(|/.*)$. Multiple apps importStorageServicefrom@credebl/storage, so Jest can fail with unresolved module errors.Suggested fix
"moduleNameMapper": { + "^`@credebl/storage`(|/.*)$": "<rootDir>/libs/storage/src/$1", "`@credebl/common/`(.*)": "<rootDir>/libs/common/src/$1", "`@credebl/common`": "<rootDir>/libs/common/src", "`@credebl/keycloak-url/`(.*)": "<rootDir>/libs/keycloak-url/src/$1", "`@credebl/keycloak-url`": "<rootDir>/libs/keycloak-url/src", "`@credebl/client-registration/`(.*)": "<rootDir>/libs/client-registration/src/$1", "`@credebl/client-registration`": "<rootDir>/libs/client-registration/src", "^`@credebl/prisma`(|/.*)$": "<rootDir>/libs/prisma/src/$1", "^`@credebl/repositories`(|/.*)$": "<rootDir>/libs/repositories/src/$1", "^`@credebl/user-request`(|/.*)$": "<rootDir>/libs/user-request/src/$1", "^`@credebl/enum`(|/.*)$": "<rootDir>/libs/enum/src/$1", "^`@credebl/prisma-service`(|/.*)$": "<rootDir>/libs/prisma-service/src/$1", "^`@credebl/org-roles`(|/.*)$": "<rootDir>/libs/org-roles/src/$1", "^`@credebl/user-org-roles`(|/.*)$": "<rootDir>/libs/user-org-roles/src/$1", "^y/user-activity(|/.*)$": "<rootDir>/libs/user-activity/src/$1", "^`@app/supabase`(|/.*)$": "<rootDir>/libs/supabase/src/$1", "^credebl/utility(|/.*)$": "<rootDir>/libs/utility/src/$1", "^`@credebl/config`(|/.*)$": "<rootDir>/libs/config/src/$1", "^`@credebl/context`(|/.*)$": "<rootDir>/libs/context/src/$1", "^`@credebl/logger`(|/.*)$": "<rootDir>/libs/logger/src/$1" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package.json` around lines 188 - 209, The Jest moduleNameMapper in package.json is missing the `@credebl/storage` alias, causing tests to fail to resolve imports like StorageService; add an entry to the "moduleNameMapper" object mapping the regex ^`@credebl/storage`(|/.*)$ to the corresponding source path (same style as other entries) so Jest can resolve imports from `@credebl/storage`; update the "moduleNameMapper" section (where other patterns like ^`@credebl/prisma`(|/.*)$ and ^`@credebl/logger`(|/.*)$ appear) to include the storage mapping.
🧹 Nitpick comments (3)
apps/api-gateway/src/issuance/issuance.module.ts (1)
28-28: ImportingStorageModuleisn’t required for DI correctness here; it’s optional for consistency
StorageServicehas a parameterless constructor and instantiatesLocalFsStorageService/RustFsStorageService/S3StorageServicewithnew(env-based), and those provider constructors also take no injected dependencies. This means providingStorageServicedirectly inapps/api-gateway/src/issuance/issuance.module.tsshouldn’t cause Nest DI resolution failures. ImportingStorageModulewould only be a consistency/future-proofing improvement.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api-gateway/src/issuance/issuance.module.ts` at line 28, The module currently lists StorageService directly in providers while also importing StorageModule unnecessarily; since StorageService and its concrete implementations (LocalFsStorageService, RustFsStorageService, S3StorageService) have parameterless constructors and are instantiated with new, remove the unnecessary import of StorageModule from the IssuanceModule (or alternatively explicitly import StorageModule for consistency/future-proofing) so DI remains correct; locate the providers array containing IssuanceService, CommonService, StorageService, NATSClient and either drop the StorageModule import or add the StorageModule to the module's imports for consistency.libs/storage/src/storage.service.ts (1)
12-23: ⚡ Quick winProvider selection currently bypasses Nest DI by constructing classes manually.
Using
new LocalFsStorageService()/RustFsStorageService()/S3StorageService()bypasses lifecycle/scoping and duplicates module wiring. Prefer constructor injection of providers, then select the injected instance byFILE_STORAGE_TYPE.Refactor sketch
- private storage: IStorageService; - - constructor() { + private readonly storage: IStorageService; + constructor( + private readonly localFsStorageService: LocalFsStorageService, + private readonly rustFsStorageService: RustFsStorageService, + private readonly s3StorageService: S3StorageService + ) { const storageType = process.env.FILE_STORAGE_TYPE || 'aws'; switch (storageType) { case 'local': - this.storage = new LocalFsStorageService(); + this.storage = this.localFsStorageService; break; case 'rustfs': - this.storage = new RustFsStorageService(); + this.storage = this.rustFsStorageService; break; default: - this.storage = new S3StorageService(); + this.storage = this.s3StorageService; break; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/storage/src/storage.service.ts` around lines 12 - 23, The constructor is instantiating providers directly (new LocalFsStorageService(), new RustFsStorageService(), new S3StorageService()), bypassing Nest's DI and lifecycle; change StorageService to accept the three provider instances via constructor injection (e.g., add parameters for LocalFsStorageService, RustFsStorageService, S3StorageService or use `@Inject` tokens) and in the constructor select which injected instance to assign to this.storage based on process.env.FILE_STORAGE_TYPE (or FILE_STORAGE_TYPE constant) instead of using new; keep the switch that assigns this.storage but use the injected symbols LocalFsStorageService, RustFsStorageService, S3StorageService so Nest can manage scopes and lifecycle.libs/storage/src/providers/local-fs-storage.provider.ts (1)
55-59: Correct the “undeclared AWS namespace” claim; this is mainly a consistency issue
AWS.S3.*types resolve becauseaws-sdktypings declare a globalAWSnamespace (export as namespace AWSinnode_modules/aws-sdk/index.d.ts). The real concern is consistency: this module mixesS3imports withAWS.S3.*return/param types—consider switchingPromise<S3.GetObjectOutput>/as S3.GetObjectOutputfor consistency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/storage/src/providers/local-fs-storage.provider.ts` around lines 55 - 59, The code mixes global AWS namespace types with the imported S3 type; update getFile to use the S3 type for consistency by changing the return type from Promise<AWS.S3.GetObjectOutput> to Promise<S3.GetObjectOutput> and cast the returned object as S3.GetObjectOutput (same for any other methods in this file using AWS.S3.*). Locate the getFile function in local-fs-storage.provider.ts and replace AWS.S3.* references with the imported S3.* type so all S3 types are consistent across the module.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/storage/src/providers/base-s3-storage.provider.ts`:
- Around line 31-39: Normalize the optional pathAWS before building object keys:
trim any leading/trailing slashes from pathAWS (or set it to '' if empty) and
only include the `${pathAWS}/` prefix in fileKey and the putObjectAsync Key when
pathAWS is non-empty; update the construction of fileKey and the Key passed to
putObjectAsync (which currently mirror each other) to use the normalized pathAWS
so keys become `filename-<ts>.ext` when no path is provided and
`path/filename-<ts>.ext` otherwise, keeping the existing
encodeURIComponent(filename), timestamp, and extension logic and continuing to
use putObjectAsync and this.s4.putObject.
- Around line 50-55: The uploadCsvFile method currently coerces any body via
body.toString(), which can turn plain objects into "[object Object]" and corrupt
CSVs; update uploadCsvFile to validate that the body is either a string or a
Buffer before creating the PutObjectRequest (reject or throw for other types),
and ensure the PutObjectRequest.Body uses the original string or Buffer value;
reference the uploadCsvFile function and the PutObjectRequest construction to
locate where to add the type check and error handling.
In `@libs/storage/src/providers/local-fs-storage.provider.ts`:
- Around line 47-53: The functions uploadCsvFile, getFile, deleteFile, and
storeObject are vulnerable to path traversal because they join user-supplied
keys into paths directly; update each to resolve the target path using the
existing resolveUnderBase helper (or equivalent) so the final file path is
guaranteed under the bucket directory before any fs ops. Specifically: compute
bucketDir as now, then call resolveUnderBase(bucketDir, key) (and for
storeObject also for objFilePath) to get the safe filePath, use path.dirname on
that safe path for ensureDir, and perform write/read/unlink on the resolved path
instead of path.join(bucketDir, key). Ensure this pattern is applied
consistently in uploadCsvFile, getFile, deleteFile, and storeObject.
- Around line 36-43: The bug is that when pathAWS is the empty string the
template `${pathAWS}/${encodeURIComponent(filename)}-...` produces a leading
slash, so resolveUnderBase(bucketDir, fileKey) escapes bucketDir and throws; fix
by normalizing/building the key without introducing a leading slash: trim any
leading/trailing slashes from pathAWS and only prepend it plus a slash when
non-empty (or use path.posix.join to join pathAWS and the filename component),
ensure the same normalized pathAWS (or joined path) is used for the ensureDir
call and for fileKey, and keep resolveUnderBase(bucketDir, fileKey) unchanged so
it will resolve inside bucketDir.
---
Outside diff comments:
In `@package.json`:
- Around line 188-209: The Jest moduleNameMapper in package.json is missing the
`@credebl/storage` alias, causing tests to fail to resolve imports like
StorageService; add an entry to the "moduleNameMapper" object mapping the regex
^`@credebl/storage`(|/.*)$ to the corresponding source path (same style as other
entries) so Jest can resolve imports from `@credebl/storage`; update the
"moduleNameMapper" section (where other patterns like ^`@credebl/prisma`(|/.*)$
and ^`@credebl/logger`(|/.*)$ appear) to include the storage mapping.
---
Nitpick comments:
In `@apps/api-gateway/src/issuance/issuance.module.ts`:
- Line 28: The module currently lists StorageService directly in providers while
also importing StorageModule unnecessarily; since StorageService and its
concrete implementations (LocalFsStorageService, RustFsStorageService,
S3StorageService) have parameterless constructors and are instantiated with new,
remove the unnecessary import of StorageModule from the IssuanceModule (or
alternatively explicitly import StorageModule for consistency/future-proofing)
so DI remains correct; locate the providers array containing IssuanceService,
CommonService, StorageService, NATSClient and either drop the StorageModule
import or add the StorageModule to the module's imports for consistency.
In `@libs/storage/src/providers/local-fs-storage.provider.ts`:
- Around line 55-59: The code mixes global AWS namespace types with the imported
S3 type; update getFile to use the S3 type for consistency by changing the
return type from Promise<AWS.S3.GetObjectOutput> to Promise<S3.GetObjectOutput>
and cast the returned object as S3.GetObjectOutput (same for any other methods
in this file using AWS.S3.*). Locate the getFile function in
local-fs-storage.provider.ts and replace AWS.S3.* references with the imported
S3.* type so all S3 types are consistent across the module.
In `@libs/storage/src/storage.service.ts`:
- Around line 12-23: The constructor is instantiating providers directly (new
LocalFsStorageService(), new RustFsStorageService(), new S3StorageService()),
bypassing Nest's DI and lifecycle; change StorageService to accept the three
provider instances via constructor injection (e.g., add parameters for
LocalFsStorageService, RustFsStorageService, S3StorageService or use `@Inject`
tokens) and in the constructor select which injected instance to assign to
this.storage based on process.env.FILE_STORAGE_TYPE (or FILE_STORAGE_TYPE
constant) instead of using new; keep the switch that assigns this.storage but
use the injected symbols LocalFsStorageService, RustFsStorageService,
S3StorageService so Nest can manage scopes and lifecycle.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 108666eb-25c5-4d82-a24e-dc7c994db6c7
📒 Files selected for processing (36)
.env.demo.gitignoreapps/api-gateway/src/issuance/issuance.controller.tsapps/api-gateway/src/issuance/issuance.module.tsapps/api-gateway/src/organization/organization.module.tsapps/api-gateway/src/user/user.controller.tsapps/api-gateway/src/user/user.module.tsapps/api-gateway/src/webhook/webhook.module.tsapps/api-gateway/src/x509/x509.module.tsapps/issuance/src/issuance.module.tsapps/issuance/src/issuance.service.tsapps/organization/src/organization.module.tsapps/organization/src/organization.service.tsapps/user/src/fido/fido.module.tsapps/user/src/user.module.tsapps/user/src/user.service.tsapps/utility/src/utilities.module.tsapps/utility/src/utilities.service.tslibs/aws/src/aws.module.tslibs/aws/src/aws.service.spec.tslibs/aws/src/index.tslibs/storage/package.jsonlibs/storage/src/index.tslibs/storage/src/providers/base-s3-storage.provider.tslibs/storage/src/providers/index.tslibs/storage/src/providers/local-fs-storage.provider.tslibs/storage/src/providers/rustfs-storage.provider.tslibs/storage/src/providers/s3-storage.provider.tslibs/storage/src/storage.interface.tslibs/storage/src/storage.module.tslibs/storage/src/storage.service.tslibs/storage/tsconfig.build.jsonlibs/storage/tsconfig.jsonnest-cli.jsonpackage.jsontsconfig.json
💤 Files with no reviewable changes (4)
- libs/aws/src/aws.service.spec.ts
- libs/aws/src/aws.module.ts
- libs/aws/src/index.ts
- nest-cli.json
✅ Files skipped from review due to trivial changes (7)
- libs/storage/tsconfig.build.json
- libs/storage/src/index.ts
- libs/storage/src/providers/index.ts
- libs/storage/package.json
- libs/storage/src/storage.interface.ts
- apps/api-gateway/src/issuance/issuance.controller.ts
- apps/organization/src/organization.service.ts
🛑 Comments failed to post (4)
libs/storage/src/providers/base-s3-storage.provider.ts (2)
31-39:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize optional
pathAWSbefore building object keys.Line 31 makes
pathAWSoptional, but Line 35 and Line 39 always prefix/. That produces keys like/name-<ts>.extwhen no path is provided and creates cross-provider inconsistencies.Suggested fix
- const fileKey = `${pathAWS}/${encodeURIComponent(filename)}-${timestamp}.${ext}`; + const normalizedPath = pathAWS.replace(/^\/+|\/+$/g, ''); + const fileKey = `${normalizedPath ? `${normalizedPath}/` : ''}${encodeURIComponent(filename)}-${timestamp}.${ext}`; @@ - Key: `${pathAWS}/${encodeURIComponent(filename)}-${timestamp}.${ext}`, + Key: fileKey,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/storage/src/providers/base-s3-storage.provider.ts` around lines 31 - 39, Normalize the optional pathAWS before building object keys: trim any leading/trailing slashes from pathAWS (or set it to '' if empty) and only include the `${pathAWS}/` prefix in fileKey and the putObjectAsync Key when pathAWS is non-empty; update the construction of fileKey and the Key passed to putObjectAsync (which currently mirror each other) to use the normalized pathAWS so keys become `filename-<ts>.ext` when no path is provided and `path/filename-<ts>.ext` otherwise, keeping the existing encodeURIComponent(filename), timestamp, and extension logic and continuing to use putObjectAsync and this.s4.putObject.
50-55:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent silent CSV payload corruption from generic
toString()conversion.Line 54 can serialize plain objects to
"[object Object]", which silently stores invalid content. Validate payload type (string | Buffer) before upload.Suggested fix
async uploadCsvFile(key: string, body: unknown): Promise<void> { + if (!(typeof body === 'string' || Buffer.isBuffer(body))) { + throw new RpcException('CSV body must be a string or Buffer'); + } const params: AWS.S3.PutObjectRequest = { Bucket: process.env.FILE_SHARING_BUCKET, Key: key, - Body: 'string' === typeof body ? body : body.toString() + Body: body };🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 54-54: 'body' will use Object's default stringification format ('[object Object]') when stringified.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/storage/src/providers/base-s3-storage.provider.ts` around lines 50 - 55, The uploadCsvFile method currently coerces any body via body.toString(), which can turn plain objects into "[object Object]" and corrupt CSVs; update uploadCsvFile to validate that the body is either a string or a Buffer before creating the PutObjectRequest (reject or throw for other types), and ensure the PutObjectRequest.Body uses the original string or Buffer value; reference the uploadCsvFile function and the PutObjectRequest construction to locate where to add the type check and error handling.Source: Linters/SAST tools
libs/storage/src/providers/local-fs-storage.provider.ts (2)
36-43:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winDefault
pathAWSbuilds an absolute-style key and breaks the default upload path.When
pathAWSis''(default),fileKeybecomes/<name>-<ts>.<ext>. On Line 42,resolveUnderBasethen resolves outsidebucketDirand throwsBAD_REQUEST, so the default call path fails.Proposed fix
- const fileKey = `${pathAWS}/${encodeURIComponent(filename)}-${timestamp}.${ext}`; + const normalizedPrefix = pathAWS ? `${pathAWS.replace(/^\/+|\/+$/g, '')}/` : ''; + const fileKey = `${normalizedPrefix}${encodeURIComponent(filename)}-${timestamp}.${ext}`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.pathAWS: string = '' ): Promise<string> { const bucketDir = path.join(this.localStoragePath, bucketName); await this.ensureDir(path.join(bucketDir, pathAWS)); const timestamp = Date.now(); const normalizedPrefix = pathAWS ? `${pathAWS.replace(/^\/+|\/+$/g, '')}/` : ''; const fileKey = `${normalizedPrefix}${encodeURIComponent(filename)}-${timestamp}.${ext}`; const filePath = this.resolveUnderBase(bucketDir, fileKey); await fs.writeFile(filePath, fileBuffer);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/storage/src/providers/local-fs-storage.provider.ts` around lines 36 - 43, The bug is that when pathAWS is the empty string the template `${pathAWS}/${encodeURIComponent(filename)}-...` produces a leading slash, so resolveUnderBase(bucketDir, fileKey) escapes bucketDir and throws; fix by normalizing/building the key without introducing a leading slash: trim any leading/trailing slashes from pathAWS and only prepend it plus a slash when non-empty (or use path.posix.join to join pathAWS and the filename component), ensure the same normalized pathAWS (or joined path) is used for the ensureDir call and for fileKey, and keep resolveUnderBase(bucketDir, fileKey) unchanged so it will resolve inside bucketDir.
47-53:
⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPath traversal protections are missing on non-upload-file operations.
uploadCsvFile,getFile,deleteFile, andstoreObjectjoin keys directly into filesystem paths withoutresolveUnderBase. A crafted key like../../...can escape the bucket directory and enable arbitrary file read/write/delete.Proposed fix pattern
- const filePath = path.join(bucketDir, key); + const filePath = this.resolveUnderBase(bucketDir, key);Apply the same pattern consistently for
getFile,deleteFile, andstoreObject(includingobjFilePath).Also applies to: 55-64, 66-74
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 51-51: 'body' will use Object's default stringification format ('[object Object]') when stringified.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/storage/src/providers/local-fs-storage.provider.ts` around lines 47 - 53, The functions uploadCsvFile, getFile, deleteFile, and storeObject are vulnerable to path traversal because they join user-supplied keys into paths directly; update each to resolve the target path using the existing resolveUnderBase helper (or equivalent) so the final file path is guaranteed under the bucket directory before any fs ops. Specifically: compute bucketDir as now, then call resolveUnderBase(bucketDir, key) (and for storeObject also for objFilePath) to get the safe filePath, use path.dirname on that safe path for ensureDir, and perform write/read/unlink on the resolved path instead of path.join(bucketDir, key). Ensure this pattern is applied consistently in uploadCsvFile, getFile, deleteFile, and storeObject.
Contributor
|
@ankita-p17 - Can you also go through the PR changes? |
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
2 participants
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.
What
Summary by CodeRabbit
New Features
Configuration