Show full location (including parents) of items in search results #347
Conversation
WalkthroughThe changes in this pull request enhance the Changes
Assessment against linked issues
Possibly related PRs
Security Recommendations
Warning Rate limit exceeded@Tom-stack3 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 6 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📜 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 (
|
Tom-stack3
force-pushed
the
enhancement/show-parent-locations
branch
from
43e9794
to
8afa69f
Compare
![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: 2
🧹 Outside diff range and nitpick comments (6)
frontend/components/Item/Card.vue (2)
70-74: Add validation for locationFlatTree propWhile the prop is correctly typed, consider adding runtime validation to ensure the array elements contain the expected properties, especially
idandtreeStringwhich are used in the computed property.Consider adding a validator:
locationFlatTree: { type: Array as () => FlatTreeItem[], required: true, + validator: (value: FlatTreeItem[]) => { + return value.every(item => + typeof item.id === 'string' && + typeof item.treeString === 'string' + ); + } },
Line range hint
46-51: Review security considerations for dynamic URLsSeveral security considerations for URL handling:
- Image URL construction should validate
item.idanditem.imageId- Navigation links should validate IDs before routing
- Consider implementing Content Security Policy (CSP) headers
Consider implementing URL validation:
const imageUrl = computed(() => { if (!props.item.imageId) { return "/no-image.jpg"; } + // Validate IDs to prevent path traversal + const itemId = validateId(props.item.id); + const imageId = validateId(props.item.imageId); + if (!itemId || !imageId) return "/no-image.jpg"; - return api.authURL(`/items/${props.item.id}/attachments/${props.item.imageId}`); + return api.authURL(`/items/${itemId}/attachments/${imageId}`); });frontend/pages/items.vue (4)
Line range hint
63-93: Add input validation for URL parametersThe current implementation directly uses query parameters without proper validation. This could lead to XSS vulnerabilities if malicious data is injected into the URL.
Consider adding validation:
const qLoc = route.query.loc as string[]; if (qLoc) { + // Validate location IDs format and existence + const validLocations = qLoc.every(id => /^[a-zA-Z0-9-]+$/.test(id)); + if (!validLocations) { + console.error('Invalid location IDs detected'); + return; + } selectedLocations.value = locations.value.filter(l => qLoc.includes(l.id)); }
Line range hint
284-331: Enhance search function securityThe search function accepts user input without proper sanitization and lacks rate limiting.
Consider implementing:
- Input sanitization
- Rate limiting
- Maximum query length validation
async function search() { if (searchLocked.value) { return; } + // Add rate limiting + if (!canMakeRequest()) { + toast.error("Too many requests. Please wait."); + return; + } + // Validate query length + if (query.value && query.value.length > 100) { + toast.error("Search query too long"); + return; + } loading.value = true;
Line range hint
333-368: Secure URL parameter handlingThe current implementation allows direct manipulation of URL parameters without proper sanitization.
Add validation before pushing to router:
async function submit() { const fields = []; for (const t of fieldTuples.value) { - if (t[0] && t[1]) { + if (t[0] && t[1] && isValidFieldValue(t[0], t[1])) { fields.push(`${t[0]}=${t[1]}`); } } + // Sanitize values before pushing to URL + const sanitizedQuery = sanitizeInput(query.value); + const sanitizedPageSize = Math.min(100, Math.max(1, pageSize.value));
Line range hint
1-585: General Security RecommendationsConsider implementing these security measures across the component:
- Add Content Security Policy (CSP) headers
- Implement CSRF protection for API calls
- Add input length limits for all user inputs
- Implement proper error logging
- Add rate limiting for search operations
- Sanitize all user inputs before processing
- Implement proper session handling
- Add security headers (X-Frame-Options, X-XSS-Protection, etc.)
Would you like me to provide specific implementation details for any of these security measures?
🛑 Comments failed to post (2)
frontend/components/Item/Card.vue (1)
76-78: 🛠️ Refactor suggestion
Handle missing locations and implement length limiting
Two improvements needed for the
locationStringcomputed property:
- Add a fallback for when location is not found
- Implement length limiting as mentioned in the PR objectives
Consider this implementation:
const locationString = computed(() => { - return props.locationFlatTree.find(l => l.id === props.item.location.id)?.treeString; + const location = props.locationFlatTree.find(l => l.id === props.item.location.id); + if (!location?.treeString) return 'Unknown location'; + + // Limit to last 3 segments if too long + const segments = location.treeString.split(' > '); + if (segments.length > 3) { + return '... > ' + segments.slice(-3).join(' > '); + } + return location.treeString; });📝 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.const locationString = computed(() => { const location = props.locationFlatTree.find(l => l.id === props.item.location.id); if (!location?.treeString) return 'Unknown location'; // Limit to last 3 segments if too long const segments = location.treeString.split(' > '); if (segments.length > 3) { return '... > ' + segments.slice(-3).join(' > '); } return location.treeString; });frontend/pages/items.vue (1)
477-477:
⚠️ Potential issueProtect against XSS in template rendering
When passing the locationFlatTree to ItemCard, ensure proper HTML escaping is in place.
Consider using Vue's built-in XSS protection:
- <ItemCard v-for="item in items" :key="item.id" :item="item" :location-flat-tree="locationFlatTree" /> + <ItemCard + v-for="item in items" + :key="item.id" + :item="item" + :location-flat-tree="sanitizeLocationTree(locationFlatTree)" + v-bind="$attrs" + />Committable suggestion skipped: line range outside the PR's diff.
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: 2
🧹 Outside diff range and nitpick comments (2)
frontend/components/Item/Card.vue (2)
70-74: Add type validation for locationFlatTree propThe prop definition looks good, but consider adding runtime validation to ensure data integrity.
Consider adding validator function:
locationFlatTree: { type: Array as () => FlatTreeItem[], required: true, + validator: (value: FlatTreeItem[]) => { + return value.every(item => + typeof item.id === 'string' && + typeof item.treeString === 'string' + ); + } },
Line range hint
1-90: Security Review: URL Construction and NavigationThe component uses several dynamic URLs that could be security-sensitive:
/item/${item.id}/location/${item.location.id}- Image URL construction in
imageUrlcomputed propertyEnsure that:
- The IDs are properly validated before being used in URLs
- The
authURLhelper implements proper URL encoding- Consider implementing URL sanitization middleware
🛑 Comments failed to post (2)
frontend/components/Item/Card.vue (2)
76-77:
⚠️ Potential issueAdd null check for item.location
The computed property needs defensive programming to handle cases where
item.locationmight be null.Apply this fix:
const locationString = computed(() => { - return props.locationFlatTree.find(l => l.id === props.item.location.id)?.treeString || props.item.location.name; + if (!props.item.location) return ''; + return props.locationFlatTree.find(l => l.id === props.item.location.id)?.treeString || props.item.location.name; });📝 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.const locationString = computed(() => { if (!props.item.location) return ''; return props.locationFlatTree.find(l => l.id === props.item.location.id)?.treeString || props.item.location.name;
17-17:
⚠️ Potential issueEnsure XSS protection for location display
While the change from
item.location.nametolocationStringimproves functionality, we should ensure proper sanitization of the location string before rendering.Consider using Vue's built-in escaping or a sanitization library:
- {{ locationString }} + {{ $sanitize(locationString) }}Committable suggestion skipped: line range outside the PR's diff.
|
I see that the Lint check has failed in the typecheck stage, but I don't really understand the output of it. So I'm not sure how to fix it :( |
|
Looks like an issue with a typescript update breaking vue-tsc, vuejs/language-tools#5018. I think just ignore it for now @tankerkiller125? |
|
Well, this seems to have broken all other ItemCard component instances except the search page since locationFlatTree isn't set there. Need to make locationFlatTree optional in Card.vue and use withDefaults: type Props = {
item: ItemOut | ItemSummary;
locationFlatTree?: FlatTreeItem[];
};
//......
const props = withDefaults(defineProps<Props>(), {
locationFlatTree: () => [],
}); |
|
Somehow, I didn't catch that, and the tests (mostly) all passed. That's for the quick fix I'll get that sorted here in a bit. |
Sets the locationFlatTree to be an optional prop for the Card component. With a default of an empty array.
|
Sadly, I missed it. Sorry for the trouble, and thanks for the fix @adamkleizer @tankerkiller125 :) |
What type of PR is this?
What this PR does / why we need it:
locationFlatTreefromfrontend/pages/items.vuetofrontend/components/Item/Card.vuein order to display full location (including parents).item.location.nameas a fallback in case the location wasn't found inlocationFlatTreeWhich issue(s) this PR fixes:
Fixes #344
Special notes for your reviewer:
Maybe need to think on limiting the length displayed. If the chain is too long we might consider shortening it in some way, or just show the location without its parents. Wasn't sure what to decide, so left it to review.
Testing
Created items under nested locations, and saw that it looks fine.
Before
After
Summary by CodeRabbit
New Features
locationFlatTreeprop.Bug Fixes
Documentation