Skip to content
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

Get front end tests passing #299

Merged
merged 13 commits into from
Oct 28, 2024
Merged

Conversation

tonyaellie
Copy link
Collaborator

@tonyaellie tonyaellie commented Oct 25, 2024

What type of PR is this?

(REQUIRED)

  • cleanup

What this PR does / why we need it:

(REQUIRED)

  • Mostly sorts issues with linting and fixes one of the tests.

Summary by CodeRabbit

Release Notes

  • New Features

    • Enhanced styling for markdown elements, including headings and lists.
    • Improved scrollbar appearance with custom styles.
    • Dynamic HTML content added to the tools page for better user guidance.
    • New computed properties for better management of item details and user interactions.
  • Bug Fixes

    • Streamlined error handling during item creation and updates to prevent multiple submissions.
    • Adjusted conditions for displaying character counts in text fields and areas.
    • Improved handling of asynchronous data fetching for item retrieval.
  • Documentation

    • Improved code readability and structure across various components, enhancing maintainability.
  • Style

    • Reordered class attributes for better CSS specificity and rendering order across multiple components.
    • Reformatted components for improved readability, including FormTextArea, FormTextField, and various modals.
  • Chores

    • Cleaned up unused styles and improved formatting in several files for consistency.
    • Added a new dependency to enhance development capabilities.
    • Updated type checking script in package configuration.

@tonyaellie
Copy link
Collaborator Author

tonyaellie commented Oct 25, 2024

Still need to test none of these changes have caused any bugs, also need to sort the typecheck.

Copy link
Contributor

coderabbitai bot commented Oct 25, 2024

Walkthrough

The pull request introduces a series of modifications across various Vue components and CSS files. Key updates include new styles for the .scroll-bg class and adjustments to the .markdown class in the CSS file. Several components, including SectionHeader.vue, Autocomplete2.vue, and TextArea.vue, have undergone logic refinements, particularly regarding length validation and class bindings. Additionally, template formatting has been improved for readability across multiple components, while some computed properties have been added or modified for better data handling.

Changes

File Path Change Summary
frontend/assets/css/main.css Added styles for .scroll-bg and .markdown class elements, including margins and heading styles.
frontend/components/Base/SectionHeader.vue Removed conditional class binding for h3 element based on dark prop.
frontend/components/Form/Autocomplete2.vue Updated computedItems logic for strict type checking and modernized string inclusion method.
frontend/components/Form/TextArea.vue Modified length validation logic for maxLength and minLength, updated default values to -1.
frontend/components/Form/TextField.vue Similar updates to length validation logic and default values for maxLength and minLength.
frontend/components/Item/Card.vue Reordered class attributes in the <h2> element.
frontend/components/Item/CreateModal.vue Improved formatting for FormTextArea and ul elements; added location check in create function.
frontend/components/Item/View/Table.vue Enhanced header handling with :key binding and improved clarity in defaultHeaders.
frontend/components/Label/CreateModal.vue Reformatted FormTextArea for readability.
frontend/components/Location/CreateModal.vue Reformatted elements for improved readability; no functional changes.
frontend/components/Location/Selector.vue Reformatted attributes for readability; no functional changes.
frontend/components/Location/Tree/Root.vue Added ESLint directive to disable specific rule; no functional changes.
frontend/components/Maintenance/ListView.vue Reordered class attributes for StatCard and button elements.
frontend/components/global/Markdown.vue Added ESLint directive for v-html and removed CSS styles affecting .markdown class.
frontend/composables/use-formatters.ts Streamlined formatting functions and updated fmtDate signature.
frontend/layouts/default.vue Reordered class attributes in various elements for styling purposes.
frontend/lib/api/__test__/user/items.test.ts Updated test assertions for maintenance logs; removed checks for cost metrics.
frontend/pages/item/[id]/index.vue Modified computed properties to enhance item detail management and user interaction.
frontend/pages/item/[id]/index/edit.vue Improved formatting in template bindings; no functional changes.
frontend/pages/label/[id].vue Reformatted FormTextArea and BaseButton for consistency; no functional changes.
frontend/pages/location/[id].vue Reformatted components for readability; streamlined update and delete functionalities.
frontend/pages/profile.vue Removed useI18n import; adjusted language selection logic for clarity.
frontend/pages/tools.vue Added HTML content with v-html for dynamic descriptions in import/export and actions sections.
frontend/package.json Added dependency @vue/runtime-core with version ^3.5.12 in devDependencies.

Possibly related PRs

Suggested reviewers

  • tankerkiller125
  • katosdev

Security Recommendations

  • Ensure that any user-generated content rendered with v-html in the Markdown.vue and tools.vue components is properly sanitized to prevent XSS attacks.
  • Review the handling of user input in the TextArea.vue and TextField.vue components to ensure that validation is robust and prevents injection attacks.

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between a558147 and 8b020a3.

📒 Files selected for processing (1)
  • frontend/pages/reports/label-generator.vue (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • frontend/pages/reports/label-generator.vue

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.

Actionable comments posted: 9

🧹 Outside diff range and nitpick comments (7)
frontend/components/global/Markdown.vue (1)

27-28: Security considerations for v-html usage

While the ESLint disable comment is necessary here, the implementation is secure because:

  1. Input is sanitized through DOMPurify before rendering
  2. The class scope is properly limited to 'markdown'

However, consider adding a comment explaining the security measures in place:

-  <!-- eslint-disable-next-line vue/no-v-html -->
+  <!-- eslint-disable-next-line vue/no-v-html -->
+  <!-- Security: Content is sanitized via DOMPurify before rendering -->
   <div class="markdown text-wrap" v-html="raw"></div>
frontend/assets/css/main.css (1)

32-39: LGTM! Consider making scrollbar width configurable.

The scrollbar styling is well-implemented. Consider making the width configurable via a CSS variable for better maintainability.

+:root {
+  --scrollbar-width: 0.5rem;
+}
 .scroll-bg::-webkit-scrollbar {
-  width: 0.5rem;
+  width: var(--scrollbar-width);
 }
frontend/components/Form/TextArea.vue (1)

Line range hint 77-82: Implement safeguards for auto-grow functionality

The auto-grow feature could be exploited for DoS attacks by submitting large amounts of text or newlines. Consider:

  1. Adding a maximum height limit
  2. Implementing rate limiting for resize operations
  3. Adding a maximum resize iterations check

Add height limits:

 function setHeight() {
+  const MAX_HEIGHT = 500; // pixels
   el.value.style.height = "auto";
-  el.value.style.height = el.value.scrollHeight + 5 + "px";
+  const newHeight = Math.min(el.value.scrollHeight + 5, MAX_HEIGHT);
+  el.value.style.height = `${newHeight}px`;
 }
frontend/components/Form/TextField.vue (1)

Line range hint 1-100: Security Advisory: Add CSRF Protection

While the component handles basic input validation, it should be used within a form that implements CSRF protection.

Ensure the parent form includes:

  1. CSRF tokens
  2. Rate limiting
  3. Input validation at the API level

Consider implementing these security headers in your application:

  • Content-Security-Policy
  • X-Frame-Options
  • X-XSS-Protection
frontend/composables/use-formatters.ts (1)

86-86: Approve explicit return type and suggest additional validation

The explicit string return type improves type safety. However, consider adding input validation for the format parameter.

Apply this diff to add format validation:

-export function fmtDate(value: string | Date, fmt: DateTimeFormat = "human"): string {
+export function fmtDate(value: string | Date, fmt: DateTimeFormat = "human"): string {
+  const validFormats: DateTimeFormat[] = ["relative", "long", "short", "human"];
+  if (!validFormats.includes(fmt)) {
+    console.warn(`Invalid format: ${fmt}. Defaulting to "human"`);
+    fmt = "human";
+  }
frontend/components/Form/Autocomplete2.vue (1)

Line range hint 1-190: Security Recommendations

Several security considerations for this component:

  1. Input Validation:

    • Add input length limits to prevent DoS attacks
    • Validate search input format
  2. XSS Prevention:

    • Ensure all displayed values are properly escaped
    • Use v-text instead of v-html where possible
  3. Object Property Access:

    • Add type checking before accessing properties
    • Consider using optional chaining

Consider implementing these security enhancements:

+ const MAX_SEARCH_LENGTH = 100;
+ const sanitizedSearch = search.value.slice(0, MAX_SEARCH_LENGTH);

+ // Type guard for item access
+ function isValidItem(item: unknown): item is { id: number, treeString: string } {
+   return typeof item === 'object' 
+     && item !== null 
+     && 'id' in item 
+     && 'treeString' in item;
+ }
frontend/pages/item/[id]/index.vue (1)

Security Vulnerabilities Confirmed - Action Required

Based on the code analysis, several critical security issues need immediate attention:

  1. Markdown Content:

    • Raw markdown content is rendered without sanitization (<Markdown :source="item.description">)
    • No HTML sanitization implementation found in the codebase
  2. File Attachments:

    • Direct URL construction for attachments using api.authURL() without proper path validation
    • Missing file type validation for uploads
    • No Content-Security-Policy headers for attachment rendering
  3. Authentication:

    • Basic auth middleware present but lacks comprehensive security checks
    • Token-based authentication exists but CSRF protection is missing for state-changing operations

Required Changes:

  • Implement DOMPurify or similar HTML sanitization for markdown rendering
  • Add file type validation in dropAttachment handler
  • Implement proper CSRF protection for attachment operations
  • Add Content-Security-Policy headers for attachment downloads
  • Add rate limiting for API operations
🔗 Analysis chain

Line range hint 1-24: Security Review: Address potential vulnerabilities

Several security considerations should be addressed:

  1. XSS Prevention:

    • Ensure the Markdown renderer sanitizes HTML content
    • Validate and sanitize user input in item.description
  2. File Security:

    • Implement proper access controls for attachment downloads
    • Validate file paths to prevent path traversal
    • Add Content-Security-Policy headers for attachment rendering
  3. Data Protection:

    • Consider masking sensitive data (serial numbers, asset IDs)
    • Implement rate limiting for API calls
    • Add audit logging for sensitive data access
  4. State Changes:

    • Add CSRF protection for quantity adjustment
    • Implement proper validation for all user inputs

Let's verify the markdown sanitization:

Check for proper CSRF protection:

Also applies to: 237-237


Let me search for more specific patterns to verify the security concerns:


Let me also check for specific security-related implementations:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if markdown sanitization is implemented
rg -l "sanitize|DOMPurify|markdownit" --type vue

Length of output: 78


Script:

#!/bin/bash
# Look for CSRF token usage in API calls
rg -A 5 "api\.(items|attachments)\.(patch|post|put|delete)"

Length of output: 1288


Script:

#!/bin/bash
# Check for markdown rendering components and sanitization
rg -A 5 "v-html|markdown|description" --type typescript --type javascript --type vue

# Check for file download and attachment handling
rg -A 5 "download|attachment" --type typescript --type javascript --type vue

# Check for API security configurations
rg -A 5 "csrf|token|headers" frontend/lib/api/

Length of output: 9278


Script:

#!/bin/bash
# Check for input validation and sanitization
rg -A 5 "validate|sanitize|clean|escape" --type typescript --type javascript --type vue

# Look for Content-Security-Policy implementations
rg -A 5 "Content-Security-Policy|CSP" 

# Check for rate limiting implementations
rg -A 5 "rateLimit|throttle|limit" frontend/lib/api/

Length of output: 702


Script:

#!/bin/bash
# Check for markdown rendering implementation
rg -p "markdown|description" frontend/pages/item/

# Check for file attachment handling
rg -p "attachment|download" frontend/pages/item/

# Check for API security implementation in the frontend
rg -p "Authorization|Bearer|headers" frontend/lib/api/base/

Length of output: 4880


Script:

#!/bin/bash
# Look for authentication middleware
rg -p "auth|middleware" frontend/middleware/

# Check for API request implementations
rg -p "fetch|axios|request" frontend/lib/api/base/ -A 10

# Look for file type validation
rg -p "mimetype|fileType|contentType" frontend/

Length of output: 1208

🛑 Comments failed to post (9)
frontend/components/Form/TextArea.vue (2)

12-12: 🛠️ Refactor suggestion

Refactor complex template expressions into computed properties

The length display logic contains complex inline expressions that could be moved to computed properties for better maintainability and testing.

Consider this refactoring:

+ const showLengthInfo = computed(() => 
+   typeof value.value === "string" && (props.maxLength !== -1 || props.minLength !== -1)
+ );
+ const lengthDisplay = computed(() => 
+   showLengthInfo.value ? `${value.value.length}/${props.maxLength}` : ""
+ );

- {{ typeof value === "string" && (maxLength !== -1 || minLength !== -1) ? `${value.length}/${maxLength}` : "" }}
+ {{ lengthDisplay }}

Also applies to: 27-27


66-66: ⚠️ Potential issue

Enhance prop validation for better security

The current prop validation only checks types. Consider adding custom validators to ensure:

  1. maxLength is greater than minLength when both are set
  2. Neither can be zero or negative (except -1)
  3. Values are within reasonable bounds to prevent DoS attacks

Add custom validators:

 maxLength: {
   type: Number,
   default: -1,
   required: false,
+  validator: (value: number) => value === -1 || value > 0
 },
 minLength: {
   type: Number,
   default: -1,
   required: false,
+  validator: (value: number) => value === -1 || value > 0
 },

Also applies to: 71-71

frontend/components/Form/TextField.vue (3)

12-12: 🛠️ Refactor suggestion

Consider adding input sanitization

While the character count display logic is correct, the component could benefit from additional security measures.

Consider adding input sanitization:

 <input
   ref="input"
   v-model="value"
+  @input="sanitizeInput"
   :placeholder="placeholder"
   :type="type"
   :required="required"
   class="input input-bordered w-full"
 />

+<script setup>
+const sanitizeInput = (event: Event) => {
+  const target = event.target as HTMLInputElement;
+  // Remove potentially dangerous characters
+  target.value = target.value.replace(/[<>]/g, '');
+};
+</script>

Also applies to: 34-34


79-79: 🛠️ Refactor suggestion

Consider adding type validation for numeric inputs

The props definition changes look good, but when type="number" is used, consider adding numeric range validation.

Add validation for numeric inputs:

 props = defineProps({
   // ... existing props
+  min: {
+    type: Number,
+    default: null
+  },
+  max: {
+    type: Number,
+    default: null
+  }
 });

+const validateNumericInput = (val: string) => {
+  if (props.type === 'number' && props.min !== null && props.max !== null) {
+    const num = Number(val);
+    return num >= props.min && num <= props.max;
+  }
+  return true;
+};

Also applies to: 84-84


9-9: 💡 Codebase verification

Security Recommendation: Add Input Length Constraints to Props Definition

The TextField component's validation logic has been updated to use -1 as a sentinel value, but I notice the props definition doesn't include maxLength and minLength props with their constraints. For proper security:

  • Add explicit prop definitions for maxLength and minLength with appropriate type validation
  • Consider adding reasonable default maximum lengths to prevent potential DoS attacks
  • Document the expected length constraints in the component props
maxLength: {
  type: Number,
  default: -1,
  validator: (value) => value === -1 || (value > 0 && value <= 1000)
},
minLength: {
  type: Number,
  default: -1,
  validator: (value) => value === -1 || value >= 0
}
🔗 Analysis chain

LGTM: Length validation logic updated correctly

The validation logic has been updated to use -1 as the sentinel value, which is a cleaner approach than checking for null.

Let's verify this component's usage to ensure proper validation:

Also applies to: 31-31


Let me try a different approach to analyze the validation patterns and component usage. The previous command failed because the vue file type wasn't recognized, so I'm broadening the search to catch any relevant files and understand the validation patterns better.


I noticed the TextField component is used in multiple places. Let's examine its actual usage patterns and props validation to ensure the length validation is properly implemented and used securely across the application.


Let's try a different approach to examine the TextField component implementation and its usage patterns. The previous commands might have failed due to file path escaping issues, so I'm adjusting the search patterns and file paths.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for instances where TextField is used with length constraints
rg -l "TextField.*maxLength|TextField.*minLength" --type vue

Length of output: 90


Script:

#!/bin/bash
# Search for TextField usage patterns with different file extensions
rg -l "TextField.*maxLength|TextField.*minLength" --type-list
rg -l "TextField.*maxLength|TextField.*minLength"

# Search for TextField component usage
rg -l "TextField"

# Look for any form validation patterns
rg "maxLength.*minLength" -A 5 -B 5

Length of output: 5213


Script:

#!/bin/bash
# Let's examine how TextField is used in the identified files
rg -A 5 -B 5 "<TextField.*>" frontend/pages/location/\[id\].vue frontend/pages/profile.vue frontend/pages/index.vue frontend/pages/item/new.vue frontend/pages/label/\[id\].vue frontend/pages/items.vue frontend/pages/item/\[id\]/index/edit.vue frontend/components/Location/CreateModal.vue frontend/components/Maintenance/EditModal.vue frontend/components/Item/CreateModal.vue frontend/components/Label/CreateModal.vue frontend/components/Form/Password.vue

# Also check for any props validation in the component
rg "props:|Props:" -A 20 frontend/components/Form/TextField.vue

Length of output: 522


Script:

#!/bin/bash
# Try a different approach to find TextField usage patterns
rg -A 10 "TextField" frontend/components/Form/TextField.vue

# Look for prop definitions with a more flexible pattern
rg "defineProps|props:" -A 20 frontend/components/Form/TextField.vue

# Check for actual TextField usage in components with proper escaping
rg -A 5 -B 5 "<TextField" frontend/components/{Location,Maintenance,Item,Label}/*Modal.vue frontend/pages/{location,profile,index,items}*.vue

Length of output: 801

frontend/composables/use-formatters.ts (2)

48-49: ⚠️ Potential issue

Strengthen input validation for time formatters

The regex pattern /\d/ could match partial numbers within strings. Consider using a more strict pattern like /^\d+$/ to ensure the entire string contains only digits.

Apply this diff to improve security:

-    past: n => (n.match(/\d/) ? t("components.global.date_time.ago", [n]) : n),
-    future: n => (n.match(/\d/) ? t("components.global.date_time.in", [n]) : n),
+    past: n => (n.match(/^\d+$/) ? t("components.global.date_time.ago", [n]) : n),
+    future: n => (n.match(/^\d+$/) ? t("components.global.date_time.in", [n]) : n),
📝 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.

    past: n => (n.match(/^\d+$/) ? t("components.global.date_time.ago", [n]) : n),
    future: n => (n.match(/^\d+$/) ? t("components.global.date_time.in", [n]) : n),

74-76: ⚠️ Potential issue

Sanitize numeric inputs in time formatters

The time unit formatters directly interpolate the n parameter without validation. Consider adding type checking and range validation.

Apply this diff to improve security:

-    hour: n => `${n} ${n === 1 ? t("components.global.date_time.hour") : t("components.global.date_time.hours")}`,
-    minute: n => `${n} ${n === 1 ? t("components.global.date_time.minute") : t("components.global.date_time.minutes")}`,
-    second: n => `${n} ${n === 1 ? t("components.global.date_time.second") : t("components.global.date_time.seconds")}`,
+    hour: n => {
+      const value = Math.max(0, parseInt(String(n), 10));
+      return `${value} ${value === 1 ? t("components.global.date_time.hour") : t("components.global.date_time.hours")}`;
+    },
+    minute: n => {
+      const value = Math.max(0, parseInt(String(n), 10));
+      return `${value} ${value === 1 ? t("components.global.date_time.minute") : t("components.global.date_time.minutes")}`;
+    },
+    second: n => {
+      const value = Math.max(0, parseInt(String(n), 10));
+      return `${value} ${value === 1 ? t("components.global.date_time.second") : t("components.global.date_time.seconds")}`;
+    },
📝 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.

    hour: n => {
      const value = Math.max(0, parseInt(String(n), 10));
      return `${value} ${value === 1 ? t("components.global.date_time.hour") : t("components.global.date_time.hours")}`;
    },
    minute: n => {
      const value = Math.max(0, parseInt(String(n), 10));
      return `${value} ${value === 1 ? t("components.global.date_time.minute") : t("components.global.date_time.minutes")}`;
    },
    second: n => {
      const value = Math.max(0, parseInt(String(n), 10));
      return `${value} ${value === 1 ? t("components.global.date_time.second") : t("components.global.date_time.seconds")}`;
    },
frontend/pages/tools.vue (1)

59-59: 🛠️ Refactor suggestion

Consider using a secure HTML rendering component

Multiple instances of v-html suggest a need for a reusable secure rendering solution. Consider creating a dedicated component that handles HTML sanitization consistently.

Create a new component like this:

<!-- SafeHTML.vue -->
<template>
  <div v-html="sanitizedContent"></div>
</template>

<script setup lang="ts">
import DOMPurify from 'dompurify';

const props = defineProps<{
  content: string
}>();

const sanitizedContent = computed(() => DOMPurify.sanitize(props.content));
</script>

Then replace v-html usage with:

-<div v-html="$t('tools.actions_sub')"></div>
+<SafeHTML :content="$t('tools.actions_sub')" />

Also applies to: 77-77, 83-83

frontend/components/Item/View/Table.vue (1)

153-156: 🛠️ Refactor suggestion

Consider implementing field-level access control.

While the formatting looks good, consider implementing field-level access control for sensitive fields like purchasePrice and location based on user roles.

Example implementation:

const defaultHeaders = [
  {
    text: "items.name",
    value: "name",
    enabled: true,
    type: "name",
    requiredRole: "viewer" // Add role-based access
  },
  {
    text: "items.purchase_price",
    value: "purchasePrice",
    enabled: hasRole("finance"), // Restrict sensitive fields
    type: "price",
    requiredRole: "finance"
  }
]

frontend/components/Base/SectionHeader.vue Show resolved Hide resolved
frontend/components/Form/Autocomplete2.vue Show resolved Hide resolved
@@ -63,10 +63,12 @@
},
maxLength: {
type: Number,
default: -1,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if -1 is the best default.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely sure on this one either, either -1 or maybe Number.MAX_VALUE from Typescript?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@katosdev thoughts?

frontend/lib/api/__test__/user/items.test.ts Show resolved Hide resolved
frontend/pages/tools.vue Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (1)
frontend/package.json (1)

26-26: Security: Review dependency management practices.

While the addition of @vue/runtime-core is safe, here are some security recommendations:

  1. Consider adding package-lock.json or yarn.lock to pin all dependency versions
  2. Run regular security audits using npm audit or yarn audit
  3. Consider adding a .npmrc file with save-exact=true to prevent floating versions

Would you like me to provide a template for the .npmrc configuration?

🛑 Comments failed to post (1)
frontend/package.json (1)

26-26: 💡 Codebase verification

⚠️ Potential issue

⚠️ Version mismatch and security concerns detected

The version mismatch between Vue packages poses both compatibility and security risks:

  • vue is locked at version 3.4.8
  • @vue/runtime-core uses ^3.5.12 (allows minor updates)
  • vue-router at ^4.4.5
  • vue-i18n at ^9.14.1

Security recommendations:

  1. Lock all Vue-related core packages to the same major.minor version (3.4.x) to prevent unexpected behavior and potential security issues
  2. Remove the caret (^) from version numbers to prevent automatic minor updates that could introduce vulnerabilities
  3. Consider upgrading all Vue packages together after proper testing

Example fix in package.json:

"@vue/runtime-core": "3.4.8",
"vue": "3.4.8",
"vue-router": "4.4.5",
"vue-i18n": "9.14.1"
🔗 Analysis chain

Version mismatch with Vue core package detected.

The added @vue/runtime-core@^3.5.12 is at a higher version than the Vue core package ([email protected]). This version mismatch could potentially lead to compatibility issues.

Consider either:

  1. Upgrading Vue core to match: "vue": "^3.5.12"
  2. Or pinning runtime-core to match Vue's version: "@vue/runtime-core": "3.4.8"
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify if there are any other Vue related packages that might be affected
rg '"vue.*": ".*"' package.json

Length of output: 33


Script:

#!/bin/bash
# Check package.json content to see all Vue related dependencies
cat frontend/package.json

Length of output: 2382

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Outside diff range and nitpick comments (1)
frontend/test/vitest.config.ts (1)

6-8: Consider documenting the test setup configuration.

The globalSetup configuration is a crucial part of the test infrastructure. Consider adding a comment explaining its purpose and any important considerations for developers.

Add documentation above the test configuration:

+ /**
+  * Test configuration
+  * globalSetup: Runs before all tests to set up the test environment
+  * For security reasons, ensure the setup file doesn't execute untrusted code
+  */
  test: {
    globalSetup: "./test/setup.ts",
  },
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between c932f00 and 5283161.

⛔ Files ignored due to path filters (1)
  • frontend/pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (7)
  • frontend/components/Form/Autocomplete2.vue (5 hunks)
  • frontend/components/Maintenance/ListView.vue (1 hunks)
  • frontend/composables/use-preferences.ts (1 hunks)
  • frontend/pages/item/[id]/index/edit.vue (4 hunks)
  • frontend/pages/label/[id].vue (3 hunks)
  • frontend/pages/reports/label-generator.vue (3 hunks)
  • frontend/test/vitest.config.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • frontend/composables/use-preferences.ts
🚧 Files skipped from review as they are similar to previous changes (4)
  • frontend/components/Form/Autocomplete2.vue
  • frontend/components/Maintenance/ListView.vue
  • frontend/pages/item/[id]/index/edit.vue
  • frontend/pages/label/[id].vue
🔇 Additional comments (2)
frontend/test/vitest.config.ts (1)

5-5: ⚠️ Potential issue

Avoid using @ts-ignore.

The @ts-ignore comment suppresses TypeScript type checking, which could hide potential type-related issues. Consider fixing the underlying type error instead.

Let's check if there are other instances of ts-ignore that might indicate a pattern:

#!/bin/bash
# Search for ts-ignore usage across the codebase
rg "@ts-ignore" --type ts --type vue
frontend/pages/reports/label-generator.vue (1)

222-227: ⚠️ Potential issue

Add input validation to prevent XSS attacks

The current implementation directly renders user-provided data (item.name and item.location.name) in the template without sanitization. This could lead to XSS vulnerabilities if the API response contains malicious content.

Consider adding validation and sanitization:

 const item = allFields?.value?.items?.[i];
+const sanitizeText = (text: string) => {
+  return text.replace(/[<>]/g, '');
+};
 if (item?.location) {
+  // Validate and sanitize input
+  const sanitizedName = sanitizeText(item.name || '');
+  const sanitizedLocation = sanitizeText(item.location.name || '');
+  const sanitizedItem = {
+    name: sanitizedName,
+    location: { name: sanitizedLocation }
+  };
-  items.push(getItem(i, item as { location: { name: string }; name: string }));
+  items.push(getItem(i, sanitizedItem));
 } else {
   items.push(getItem(i, null));
 }

Additionally, consider adding runtime type validation using a schema validation library like Zod or Yup to ensure the API response matches the expected structure.

frontend/test/vitest.config.ts Show resolved Hide resolved
frontend/pages/reports/label-generator.vue Outdated Show resolved Hide resolved
frontend/pages/reports/label-generator.vue Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between f2e4732 and a842de9.

📒 Files selected for processing (3)
  • .github/workflows/partial-frontend.yaml (1 hunks)
  • frontend/lib/api/classes/items.ts (2 hunks)
  • frontend/lib/api/types/non-generated.ts (1 hunks)
🔇 Additional comments (7)
frontend/lib/api/types/non-generated.ts (1)

20-26: ⚠️ Security: Review exposure of financial data in pagination results.

The totalPrice field in pagination results could potentially expose sensitive financial information. Consider:

  1. Implementing proper access controls
  2. Adding rate limiting to prevent data scraping
  3. Evaluating if this aggregate data is necessary in pagination responses
✅ Verification successful

Let me check for authentication mechanisms and API endpoint implementations.


Let me check for authentication middleware and security configurations.


✅ Financial data is properly secured with authentication

The API endpoints that expose the ItemSummaryPaginationResult (including totalPrice) are protected by:

  1. Bearer token authentication required for all API endpoints
  2. Auth middleware that validates tokens and redirects unauthorized requests
  3. Frontend pages using the API are protected with auth middleware

The security concern about exposing financial data is addressed through proper authentication controls. However, for additional security, consider:

  • Implementing rate limiting on the API endpoints
  • Adding role-based access control for financial data
  • Logging access to sensitive financial endpoints
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for any authentication/authorization mechanisms around this API
rg -A 5 "ItemSummaryPaginationResult" 

Length of output: 1298


Script:

#!/bin/bash
# Check for authentication middleware and route handlers
rg -B 5 -A 10 "route\(\"/items\"" 

# Check for HTTP client implementation and auth headers
rg -B 5 "class.*Http" 

# Look for auth guards or middleware
ast-grep --pattern 'class $_ implements $_ {
  $$$
  get<$_>($_) {
    $$$
  }
  $$$
}'

Length of output: 1372


Script:

#!/bin/bash
# Check for auth middleware or guards
rg -B 3 -A 3 "middleware.*auth" 

# Look for security configurations
rg -B 3 -A 3 "security|authentication|authorization"

# Check for rate limiting implementations
rg -B 3 -A 3 "rateLimit|rate-limit"

# Check HTTP client configuration
ast-grep --pattern 'class Http {
  $$$
  constructor($_) {
    $$$
  }
  $$$
}'

Length of output: 110126

.github/workflows/partial-frontend.yaml (2)

Line range hint 1-52: Verify the impact of pnpm version update on the frontend tests.

The workflow changes appear correct, but since this PR aims to "Get front end tests passing", we should verify that the pnpm version update actually helps resolve the test issues.

Let's check the test failure history:

#!/bin/bash
# Description: Check recent workflow runs to see if test failures correlate with pnpm versions

# Look for previous workflow runs and their status
gh run list --workflow=".github/workflows/partial-frontend.yaml" --limit 5 --json status,conclusion,createdAt

# Check commit messages for any test-related fixes
git log --grep="test" --grep="pnpm" -i --pretty=format:"%h %s" -n 5

18-18: Security: Consider using version ranges for better maintainability.

Using fixed versions (9.12.2, 6.0.2) ensures reproducibility but might miss security updates. Consider using version ranges (e.g., ~9.12.2) to automatically get patch updates while maintaining compatibility.

Let's check if there are any security advisories for pnpm versions:

Also applies to: 52-52

frontend/lib/api/classes/items.ts (4)

15-15: LGTM: Type import change aligns with pagination enhancement

The change from PaginationResult to ItemSummaryPaginationResult reflects the enhanced pagination type that includes total price information.


101-101: LGTM: Return type updated consistently

The return type change in getAll() correctly implements the new ItemSummaryPaginationResult type.


Line range hint 32-49: Enhance file upload security measures

The file upload endpoints in AttachmentsAPI.add(), ItemsApi.import(), and related methods should implement:

  1. File type validation
  2. File size limits
  3. Antivirus scanning if possible
  4. Content-Type verification

This helps prevent malicious file uploads and potential security vulnerabilities.

#!/bin/bash
# Check for existing file validation logic
rg -A 5 "file.*validation|mime.*type|content.*type" frontend/
# Look for file size limit implementations
rg -A 5 "maxFileSize|size.*limit" frontend/

Also applies to: 124-131, 144-150


Line range hint 19-29: Consider input validation for query parameters

The ItemsQuery type allows for various query parameters that could potentially be manipulated for malicious purposes. Consider implementing:

  1. Input validation for page and pageSize to prevent DoS attacks
  2. Sanitization of q parameter to prevent injection attacks
  3. Length limits on arrays (locations, labels, parentIds)

frontend/lib/api/types/non-generated.ts Outdated Show resolved Hide resolved
.github/workflows/partial-frontend.yaml Show resolved Hide resolved
@tonyaellie tonyaellie marked this pull request as ready for review October 28, 2024 15:00
@tankerkiller125 tankerkiller125 merged commit ec5b6bb into main Oct 28, 2024
11 checks passed
@katosdev katosdev deleted the tonya/get-front-end-tests-passing branch October 29, 2024 06:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants