-
Notifications
You must be signed in to change notification settings - Fork 18
epcis plugin V1 #22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
epcis plugin V1 #22
Conversation
| } | ||
|
|
||
| // Validate against GS1 schema | ||
| const isValid = this.validateSchema(document); |
Check failure
Code scanning / CodeQL
Resources exhaustion from deep object traversal High
user input
allErrors: true
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 days ago
In general, to fix this issue you should avoid enabling allErrors: true when validating untrusted input in production. That prevents Ajv from traversing the entire object and accumulating every possible error, limiting both CPU and memory usage by stopping at the first validation failure (or at least drastically reducing the error surface). If detailed error reporting is needed in non-production environments, you can still enable allErrors conditionally based on an environment flag.
The best fix here, without changing the observable behavior too much, is to make allErrors conditional on an environment variable (for example REST_DEBUG as in the background example, or a more plugin-specific variable). In production, where that variable is not set to a truthy value, Ajv will default to reporting only the first error, mitigating the DoS risk. In development, you still get the rich error list for debugging. Concretely, in packages/plugin-epcis/src/services/EPCISValidationService.ts, update the Ajv configuration in the constructor so that allErrors is not hard-coded to true but instead reads from process.env. No other code needs to change because Ajv’s public interface (compile, validate, and errors) remains the same; only the number of errors populated in this.validateSchema.errors will change.
To implement this, you only need to change the object literal passed to new Ajv in EpcisValidationService’s constructor. No new helper methods or imports are required, as process.env is available in Node.js by default. A typical pattern is:
this.ajv = new Ajv({
allErrors: process.env.REST_DEBUG === "true",
strict: false,
validateFormats: true,
});This keeps current behavior when REST_DEBUG=true (or whatever flag you choose) and disables multi-error collection otherwise.
-
Copy modified lines R12-R14
| @@ -9,7 +9,9 @@ | ||
|
|
||
| constructor() { | ||
| this.ajv = new Ajv({ | ||
| allErrors: true, | ||
| // Avoid collecting all errors in production to prevent potential DoS from deep/large payloads | ||
| // Enable verbose error collection only when explicitly requested via environment variable | ||
| allErrors: process.env.REST_DEBUG === "true", | ||
| strict: false, | ||
| validateFormats: true, | ||
| }); |
Lexpeartha
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also document new .env variable and add it to env.d.ts
|
there is no .env variable used in code
Balki-OriginTrail
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code so beautiful, it made my cry
packages/plugin-epcis/src/index.ts
Outdated
| } as any); | ||
| } | ||
| // Query publisher for asset status | ||
| const response = await fetch(`${publisherUrl}/api/dkg/assets/status/${encodeURIComponent(captureID)}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for the future, we should avoid calling our own server from that server. Instead we should expose the publisher plugin in context
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, it would be wise to add some timeout here, so the request cannot hang forever.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do next week
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't have to rewrite the plugin to fit, it's a different task. This was just to keep in mind once it is rewritten.
Adding the timeout would be good though and it's quick
packages/plugin-epcis/src/index.ts
Outdated
| console.error("[EPCIS Status] Error:", error); | ||
| res.status(500).json({ | ||
| error: "Failed to get capture status", | ||
| message: error.message, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should return generic messages to the user and log the specific message on the server to avoid leaking sensitive data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please elaborate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This returns a full backend error to the frontend which is generally not a good practice because it exposes the backend
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while i agree, entire codebase is doing the same pattern, commented out the full error.message for now, but it's a repo wide problem which we should address in the future, will create a backlog task
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we will slowly rewrite this in the codebase, we have to start somewhere :)
the message should still exist though. Sth generic like I mentioned in the first comment. Perhaps: "Internal server error".
Also please do this for all tools in this plugin, multiple tools still return error messages
packages/plugin-epcis/src/index.ts
Outdated
| } | ||
|
|
||
| // Send to publisher | ||
| const result = await sendToPublisher(document, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be some retries here and a fallback publishing using dkg.asset.create if the publisher plugin fails
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this needs further discussion, will add it to backlog for rework
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does it need further discussion? It's a general good practice to do this
| if (params.ual) { | ||
| return this.getEventByUal(params.ual); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if someone passes ual and some exclusive fields they are interested in they will still get everything back
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also we should do ual validation here and return a proper error. Currently if the ual is not valid it just returns prefixes.
You can check how this is done in dkg.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commented out for now, isn't important for Friday demo, will be added to rework backlog
| ${filterClauses.join('\n ')} | ||
| } | ||
| ORDER BY DESC(?eventTime) | ||
| LIMIT 100`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the limit hardcoded? This should not be up to us to decide
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will be added to backlog for rework, not important for Friday
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be thinking about the general repo and others who might be using the plugin too. They won't know about this limit and could spend much time debugging why they're not getting all events back
42eaafd to
6a8e3f0
Compare
No description provided.