Fix Label Generator showing incorrect asset ID #319
Conversation
WalkthroughThe pull request involves a comprehensive reorganization of the Changes
Assessment against linked issues
Possibly related PRs
Security Recommendations
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
katosdev
added
🕷️ bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 5
🛑 Comments failed to post (5)
frontend/pages/reports/label-generator.vue (5)
332-334:
⚠️ Potential issueSecure template rendering against XSS
The template directly renders user-provided data (name and location) without sanitization, potentially allowing XSS attacks.
Recommended security measures:
- Install and use a sanitization library:
npm install xss
- Apply sanitization:
function getItem(n: number, item: { name: string; location: { name: string } } | null): LabelData { + const sanitize = require('xss'); const assetID = fmtAssetID(n); return { url: getQRCodeUrl(assetID), assetID, - name: item?.name ?? "_______________", - location: item?.location?.name ?? "_______________", + name: sanitize(item?.name ?? "_______________"), + location: sanitize(item?.location?.name ?? "_______________"), }; }
157-163: 🛠️ Refactor suggestion
Enhance error handling for API failures
The current error handling silently fails by returning an empty array. This could lead to confusion when labels are generated without data.
Suggested improvement:
const { data: allFields } = await useAsyncData(async () => { const { data, error } = await api.items.getAll(); if (error) { + notifier.error(`Failed to fetch items: ${error.message}`); return { items: [] }; } return data; });Committable suggestion skipped: line range outside the PR's diff.
166-173: 🛠️ Refactor suggestion
Strengthen input validation for asset range
The current validation allows negative numbers and has a high upper limit that could lead to performance issues.
Recommended improvements:
const items = computed(() => { + // Ensure positive numbers + if (displayProperties.assetRange < 0 || displayProperties.assetRangeMax < 0) { + notifier.error('Asset range cannot be negative'); + return []; + } + if (displayProperties.assetRange > displayProperties.assetRangeMax) { + notifier.error('Start range cannot be greater than end range'); return []; } const diff = displayProperties.assetRangeMax - displayProperties.assetRange; if (diff > 999) { + notifier.error('Range cannot exceed 999 items'); return []; }📝 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.// Ensure positive numbers if (displayProperties.assetRange < 0 || displayProperties.assetRangeMax < 0) { notifier.error('Asset range cannot be negative'); return []; } if (displayProperties.assetRange > displayProperties.assetRangeMax) { notifier.error('Start range cannot be greater than end range'); return []; } const diff = displayProperties.assetRangeMax - displayProperties.assetRange; if (diff > 999) { notifier.error('Range cannot exceed 999 items'); return []; }
138-145:
⚠️ Potential issueAddress potential XSS vulnerability in QR code URL generation
The current implementation has security concerns:
- The base URL is directly used without validation
- The asset ID is included in the URL without proper sanitization
Recommended security improvements:
function getQRCodeUrl(assetID: string): string { + // Validate base URL format + const urlPattern = /^https?:\/\/[^\s/$.?#].[^\s]*$/i; + if (!urlPattern.test(displayProperties.baseURL.trim())) { + throw new Error('Invalid base URL format'); + } + let origin = displayProperties.baseURL.trim(); if (origin.endsWith("/")) { origin = origin.slice(0, -1); } + + // Sanitize asset ID + if (!/^\d{3}-\d{3}$/.test(assetID)) { + throw new Error('Invalid asset ID format'); + } + const data = `${origin}/a/${assetID}`; return route(`/qrcode`, { data: encodeURIComponent(data) }); }Committable suggestion skipped: line range outside the PR's diff.
175-179:
⚠️ Potential issueFix asset ID ordering issue
The current implementation may contribute to the incorrect asset ID display issue mentioned in PR #289. The items array is being populated based on the index
i, but there's no guarantee that the items inallFields.value.itemsmaintain the same order as the asset IDs.Suggested fix:
- const item = allFields?.value?.items?.[i]; + const item = allFields?.value?.items?.find(item => item.id === i.toString()); items.push(getItem(i, item?.location ? item : null));Committable suggestion skipped: line range outside the PR's diff.
|
|
||
| const api = useUserApi(); | ||
|
|
||
| const bordered = ref(false); |
There was a problem hiding this comment.
This doesn't seem to be used any more.
| @@ -319,135 +257,91 @@ | |||
| <h1>Homebox Label Generator</h1> | |||
There was a problem hiding this comment.
Translation keys need to be added for all the text on this page but I think thats should be sorted in a seperate PR?
| const items: LabelData[] = []; | ||
| for (let i = displayProperties.assetRange; i < displayProperties.assetRangeMax; i++) { | ||
| const item = allFields?.value?.items?.[i]; | ||
| items.push(getItem(i, item?.location ? item : null)); |
There was a problem hiding this comment.
This is where the asset ID mismatch happens. api.items.getAll() which is used to fill allFields appears to return the items in alphabetical order based on name (seems like the entire database, which may be inefficient if you have a large number of items and only want to print a few labels). Loop index "i" is not associated with the actual assetID of allFields?.value?.items?.[i], it's just whatever order the array happened to get filled in. This loop index i is what is used to create the QR code and assetID label in "getItem". Even if the array did happen to get filled in the same order of assetID, this wouldn't handle a case where there was a gap in assetIDs.
Unfortunately it doesn't look like api.items.getAll() returns an assetID. Probably the right fix is changing that so it does, or you can look up the assetID based on the id property which is what I did in my comment on issue #289 but that seems more like a band-aid.
|
Closing in favor of #351 |
What type of PR is this?
What this PR does / why we need it:
Fixes Label Generator showing incorrect asset ID due to ordering mismatch.
Which issue(s) this PR fixes:
Fixes #289
Summary by CodeRabbit
New Features
Bug Fixes
Documentation