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

Add authentication and HTTPS #684

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Add authentication and HTTPS #684

wants to merge 15 commits into from

Conversation

KraPete
Copy link
Contributor

@KraPete KraPete commented Jan 21, 2025

echo "deb http://deb.wirenboard.com/all experimental.homeui-https main" > /etc/apt/sources.list.d/homeui-https.list

Summary by CodeRabbit

Summary by CodeRabbit

Based on the comprehensive summary, here are the updated release notes:

  • New Features

    • Added login modal for user authentication.
    • Introduced user management capabilities with a dedicated user page.
    • Enhanced role management with asynchronous rights checks.
    • Added support for environment variable configuration via .env file.
  • Improvements

    • Streamlined user interface for access management.
    • Enhanced internationalization support with new error messages and roles.
    • Improved error handling and messaging across various components.
    • Consolidated font file management into a dedicated directory.
    • Updated alert messages for insufficient permissions across various views.
  • Security

    • Implemented HTTPS redirection for enhanced security.
    • Updated role-based access controls to ensure proper permissions.
    • Removed direct access to configuration pages without authentication.
  • Bug Fixes

    • Resolved issues with user role management.
    • Improved handling of connection and login states.
  • Chores

    • Updated dependencies and configuration files, including package.json and webpack.config.js.

@KraPete KraPete requested a review from a team as a code owner January 21, 2025 16:24
Copy link

coderabbitai bot commented Jan 21, 2025

Warning

Rate limit exceeded

@KraPete has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 48 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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.

📥 Commits

Reviewing files that changed from the base of the PR and between 42dabe4 and fcb4140.

📒 Files selected for processing (2)
  • app/scripts/react-directives/components/modals/formModal.jsx (1 hunks)
  • app/scripts/react-directives/login/loginModal.jsx (1 hunks)

Walkthrough

This pull request introduces significant changes to the web application, focusing on enhancing user authentication, access control, and security. The modifications include implementing a new login modal, adding HTTPS redirection, updating role management, and improving internationalization. Key changes involve removing explicit access level management, introducing a more streamlined user management system, and adding environment variable support through dotenv. The changes aim to simplify user interactions while improving overall application security and configuration flexibility.

Changes

File/Directory Change Summary
.gitignore Added .env to ignore environment configuration files
Makefile Updated font directory creation and copying process
README.md Added instructions for changing MQTT broker URL via .env
app/index.ejs Added login modal, error alerts, and user menu
app/scripts/app.js, app/scripts/app.routes.js, app/scripts/controllers/* Removed login and access level controllers, added new directives, refactored rights checking to use asynchronous methods
app/scripts/i18n/* Updated localization strings, removed access level translations
app/views/* Modified access notice alerts across various views
app/styles/* Added new CSS classes for user menu and connection status
webpack.config.js Added dotenv support and proxy configuration
debian/* Updated package metadata and version constraints
package.json Added dotenv as a development dependency

Sequence Diagram

sequenceDiagram
    participant User
    participant LoginModal
    participant AuthService
    participant RolesFactory
    participant Application

    User->>LoginModal: Attempt Login
    LoginModal->>AuthService: Send Credentials
    AuthService->>RolesFactory: Validate User Type
    RolesFactory-->>AuthService: Return Role
    alt Login Successful
        AuthService->>Application: Grant Access
        Application->>User: Show Dashboard
    else Login Failed
        AuthService->>LoginModal: Show Error
        LoginModal->>User: Display Error Message
    end
Loading

Suggested reviewers

  • sikmir
  • ninedev-i

Poem

🐰 A Rabbit's Tale of Login and Grace

In realms of code, where secrets reside,
A modal springs forth, with security's pride.
HTTPS whispers, roles dance with glee,
No more plain paths, we're setting users free!

Hop hop hooray! 🚀


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 generate docstrings to generate docstrings for this PR. (Beta)
  • @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

@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.

🛑 Comments failed to post (59)
app/scripts/react-directives/components/select/select.jsx (1)

13-13: 🛠️ Refactor suggestion

Add PropTypes validation for the disabled prop.

Add prop type validation to ensure runtime type safety.

+ import PropTypes from 'prop-types';

const BootstrapLikeSelect = ({...}) => {
  // ... component implementation
};

+ BootstrapLikeSelect.propTypes = {
+   options: PropTypes.array.isRequired,
+   selectedOption: PropTypes.object,
+   placeholder: PropTypes.string,
+   onChange: PropTypes.func.isRequired,
+   isClearable: PropTypes.bool,
+   className: PropTypes.string,
+   disabled: PropTypes.bool
+ };

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 eslint

[error] 13-13: 'disabled' is missing in props validation

(react/prop-types)

app/scripts/react-directives/forms/stringStore.js (1)

72-75: 🧹 Nitpick (assertive)

Initialize readOnly state in constructor if not provided.

The readOnly state should have a default value when not provided in the constructor.

  constructor({ name, description, value, placeholder, validator, defaultText, readOnly }) {
    // ... other initializations
-   this.readOnly = readOnly;
+   this.readOnly = readOnly ?? false;
    // ... rest of constructor
  }

Committable suggestion skipped: line range outside the PR's diff.

app/scripts/react-directives/forms/optionsStore.js (1)

91-91: ⚠️ Potential issue

Security concern: Verify if readOnly should be reset.

The reset method unconditionally sets readOnly to false, which might allow unauthorized modifications if the form should remain locked. Consider preserving the readOnly state or adding a parameter to control this behavior.

  reset() {
    this.setValue(this.initialValue);
-   this.setReadOnly(false);
+   // Only reset readOnly if explicitly requested
+   // this.setReadOnly(false);
  }
📝 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.

  reset() {
    this.setValue(this.initialValue);
    // Only reset readOnly if explicitly requested
    // this.setReadOnly(false);
  }
app/scripts/controllers/configsController.js (2)

12-12: 🧹 Nitpick (assertive)

Add parentheses around arrow function parameter to fix ESLint warning

ESLint reports a warning regarding the missing parentheses around the arrow function parameter result at line 12. Adding parentheses improves readability and conforms to the code style guidelines.

Apply this diff to fix the issue:

-            .then(result => {
+            .then((result) => {
📝 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.

        .then((result) => {
🧰 Tools
🪛 eslint

[error] 12-12: Expected parentheses around arrow function argument.

(arrow-parens)


8-16: ⚠️ Potential issue

Ensure 'haveRights' is properly initialized when user lacks required rights

Currently, this.haveRights is set to true when the user has the required rights, but there is no explicit handling for when the user lacks the rights. This may lead to this.haveRights being undefined, which could cause unexpected behavior elsewhere in the code.

Initialize this.haveRights to false by default to ensure it has a defined state regardless of the user's rights.

Apply this diff to initialize haveRights:

 class ConfigsCtrl {
   constructor($scope, ConfigEditorProxy, whenMqttReady, errors, rolesFactory, $locale) {
     'ngInject';

     $scope.configs = [];
     $scope.$locale = $locale;
+    this.haveRights = false;

     rolesFactory.asyncCheckRights(rolesFactory.ROLE_THREE, () => {
       this.haveRights = true;
       whenMqttReady()

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 eslint

[error] 12-12: Expected parentheses around arrow function argument.

(arrow-parens)

app/scripts/controllers/devicesController.js (1)

18-21: ⚠️ Potential issue

Ensure 'haveRights' is properly initialized when user lacks required rights

Similar to other controllers, this.haveRights is set to true when the user has the required rights, but there is no explicit initialization for when the user lacks the rights. This could result in this.haveRights being undefined, leading to potential issues in the view or conditional logic.

Initialize this.haveRights to false by default.

Apply this diff to initialize haveRights:

     });
+    this.haveRights = false;

     rolesFactory.asyncCheckRights(rolesFactory.ROLE_TWO, () => {
       this.haveRights = true;
       this.createDevicesVisibilityObject();
     });

Committable suggestion skipped: line range outside the PR's diff.

app/scripts/react-directives/users/pageStore.js (8)

1-232: 🧹 Nitpick (assertive)

Refactor file to comply with 'max-classes-per-file' rule

The file contains two classes (UsersPageAccessLevelStore and UsersPageStore), exceeding the maximum allowed of one class per file according to coding standards.

Consider splitting UsersPageAccessLevelStore into a separate file for better maintainability and compliance with style guidelines.

🧰 Tools
🪛 Biome (1.9.4)

[error] 1-1: Redundant use strict directive.

The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.

(lint/suspicious/noRedundantUseStrict)


[error] 186-186: This let declares a variable that is only assigned once.

'modifiedUser' is never reassigned.

Safe fix: Use const instead.

(lint/style/useConst)

🪛 eslint

[error] 1-232: File has too many classes (2). Maximum allowed is 1.

(max-classes-per-file)


[error] 95-95: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 96-96: Expected no linebreak before this expression.

(implicit-arrow-linebreak)


[error] 97-98: Missing trailing comma.

(comma-dangle)


[error] 98-99: Missing trailing comma.

(comma-dangle)


[error] 99-99: Unexpected newline before ')'.

(function-paren-newline)


[error] 104-108: Redundant use of await on a return value.

(no-return-await)


[error] 107-108: Missing trailing comma.

(comma-dangle)


[error] 171-171: Unexpected use of 'location'.

(no-restricted-globals)


[error] 174-174: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 186-186: 'modifiedUser' is never reassigned. Use 'const' instead.

(prefer-const)


[error] 200-200: Assignment to property of function parameter 'user'.

(no-param-reassign)


[error] 201-201: Assignment to property of function parameter 'user'.

(no-param-reassign)


[error] 213-214: Missing trailing comma.

(comma-dangle)


[error] 218-218: Expected '===' and instead saw '=='.

(eqeqeq)


[error] 226-226: Expected parentheses around arrow function argument.

(arrow-parens)

🪛 GitHub Check: Codacy Static Code Analysis

[failure] 137-137: app/scripts/react-directives/users/pageStore.js#L137
This application allows user-controlled URLs to be passed directly to HTTP client libraries.


1-1: 🧹 Nitpick (assertive)

Remove redundant 'use strict' directive

In ECMAScript modules, strict mode is enabled automatically. The 'use strict' directive at the beginning of the file is unnecessary.

Apply this diff to remove the directive:

-'use strict';
🧰 Tools
🪛 Biome (1.9.4)

[error] 1-1: Redundant use strict directive.

The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.

(lint/suspicious/noRedundantUseStrict)

🪛 eslint

[error] 1-232: File has too many classes (2). Maximum allowed is 1.

(max-classes-per-file)


186-186: 🧹 Nitpick (assertive)

Use 'const' instead of 'let' for variables that are not reassigned

The variable modifiedUser is never reassigned after its initial assignment. Using const is preferred for variables that are not reassigned to improve code readability and prevent accidental reassignments.

Apply this diff:

-        let modifiedUser = await this.showUserEditModal();
+        const modifiedUser = await this.showUserEditModal();
📝 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 modifiedUser = await this.showUserEditModal();
🧰 Tools
🪛 Biome (1.9.4)

[error] 186-186: This let declares a variable that is only assigned once.

'modifiedUser' is never reassigned.

Safe fix: Use const instead.

(lint/style/useConst)

🪛 eslint

[error] 186-186: 'modifiedUser' is never reassigned. Use 'const' instead.

(prefer-const)


218-218: 🧹 Nitpick (assertive)

Use strict equality operator '===' instead of '=='

It's recommended to use the strict equality operator === to avoid type coercion issues and ensure accurate condition evaluations.

Apply this diff:

-        if ((await this.showDeleteConfirmModal(user)) == 'ok') {
+        if ((await this.showDeleteConfirmModal(user)) === 'ok') {
📝 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.

    if ((await this.showDeleteConfirmModal(user)) === 'ok') {
🧰 Tools
🪛 eslint

[error] 218-218: Expected '===' and instead saw '=='.

(eqeqeq)


171-171: 🧹 Nitpick (assertive)

Avoid using restricted global variable 'location'

Direct use of the global variable location is restricted by coding standards. To ensure clarity and avoid unintended side effects, use window.location instead.

Apply this diff:

-          location.reload();
+          window.location.reload();
📝 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.

      window.location.reload();
🧰 Tools
🪛 eslint

[error] 171-171: Unexpected use of 'location'.

(no-restricted-globals)


95-99: 🧹 Nitpick (assertive)

Fix arrow function formatting to meet style guidelines

There are formatting issues with the arrow function starting at line 95. Specifically:

  • Missing parentheses around the arrow function parameter text.
  • Unexpected line breaks and missing trailing commas.

Apply this diff to address the issues:

-            .then(text =>
-              this.pageWrapperStore.setError(
-                i18n.t('users.errors.unknown', { msg: text, interpolation: { escapeValue: false } })
-              )
-            );
+            .then((text) => {
+              this.pageWrapperStore.setError(
+                i18n.t('users.errors.unknown', { msg: text, interpolation: { escapeValue: false } })
+              );
+            });
📝 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.

          .then((text) => {
              this.pageWrapperStore.setError(
                i18n.t('users.errors.unknown', { msg: text, interpolation: { escapeValue: false } })
              );
            });
🧰 Tools
🪛 eslint

[error] 95-95: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 96-96: Expected no linebreak before this expression.

(implicit-arrow-linebreak)


[error] 97-98: Missing trailing comma.

(comma-dangle)


[error] 98-99: Missing trailing comma.

(comma-dangle)


[error] 99-99: Unexpected newline before ')'.

(function-paren-newline)


104-108: 🧹 Nitpick (assertive)

Avoid redundant use of 'await' on a return value

In the showUserEditModal method, the await keyword is used directly on the return statement, which is unnecessary.

Apply this diff to remove the redundant await:

-      async showUserEditModal() {
-        return await this.formModalState.show(
+      async showUserEditModal() {
+        return this.formModalState.show(

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 eslint

[error] 104-108: Redundant use of await on a return value.

(no-return-await)


[error] 107-108: Missing trailing comma.

(comma-dangle)


183-204: 🧹 Nitpick (assertive)

Avoid reassigning properties of function parameter 'user'

Modifying properties of function parameters is discouraged as it can lead to unintended side effects. Instead, create a new object or update the state in an immutable fashion.

Consider the following adjustment:

       if (res == null) {
         return;
       }
+      runInAction(() => {
+        const index = this.users.findIndex(u => u.id === user.id);
+        if (index !== -1) {
+          this.users[index] = {
+            ...this.users[index],
+            name: modifiedUser.name,
+            type: modifiedUser.type,
+          };
+        }
+        sortUsers(this.users);
+      });
-      user.name = modifiedUser.name;
-      user.type = modifiedUser.type;
-      sortUsers(this.users);
📝 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.

  async editUser(user) {
    this.userParamsStore.reset();
    this.userParamsStore.setValue(user);
    let modifiedUser = await this.showUserEditModal();
    if (!modifiedUser) {
      return;
    }
    const res = await this.execRequest(`/auth/users/${user.id}`, {
      method: 'PUT',
      headers: {
        'Content-Type': 'application/json',
      },
      body: JSON.stringify(modifiedUser),
    });
    if (res == null) {
      return;
    }
    runInAction(() => {
      const index = this.users.findIndex(u => u.id === user.id);
      if (index !== -1) {
        this.users[index] = {
          ...this.users[index],
          name: modifiedUser.name,
          type: modifiedUser.type,
        };
      }
      sortUsers(this.users);
    });
  }
🧰 Tools
🪛 Biome (1.9.4)

[error] 186-186: This let declares a variable that is only assigned once.

'modifiedUser' is never reassigned.

Safe fix: Use const instead.

(lint/style/useConst)

🪛 eslint

[error] 186-186: 'modifiedUser' is never reassigned. Use 'const' instead.

(prefer-const)


[error] 200-200: Assignment to property of function parameter 'user'.

(no-param-reassign)


[error] 201-201: Assignment to property of function parameter 'user'.

(no-param-reassign)

app/scripts/app.js (7)

356-363: ⚠️ Potential issue

Sanitize 'https_domain' before redirecting to prevent potential XSS vulnerabilities

Assigning a value from an external source to window.location.href without validation can lead to security issues like XSS attacks. Ensure that https_domain contains a valid and trusted domain before using it in the redirect.

Consider validating https_domain or encoding it to ensure it's safe:

-          const https_domain = await response.text();
-          response = await fetch(`https://${https_domain}/https/check`, {
-          ...
-          if (response.status === 200) {
-            window.location.href = `https://${https_domain}`;
+          const httpsDomain = await response.text();
+          // Validate the domain before using it
+          if (isValidDomain(httpsDomain)) {
+            response = await fetch(`https://${httpsDomain}/https/check`, {
+            ...
+            if (response.status === 200) {
+              window.location.href = `https://${httpsDomain}`;
+            }
+          } else {
+            console.warn('Invalid domain received:', httpsDomain);
+          }

Add a domain validation function:

function isValidDomain(domain) {
  // Implement domain validation logic
  const domainRegex = /^(?:[a-z0-9](?:[a-z0-9-]{0,61}[a-z0-9])?\.)+[a-z]{2,}$/;
  return domainRegex.test(domain);
}
🧰 Tools
🪛 eslint

[error] 356-356: Identifier 'https_domain' is not in camel case.

(camelcase)


[error] 357-357: Identifier 'https_domain' is not in camel case.

(camelcase)


[error] 362-362: Identifier 'https_domain' is not in camel case.

(camelcase)

🪛 GitHub Check: Codacy Static Code Analysis

[warning] 362-362: app/scripts/app.js#L362
Dangerous location.href assignment can lead to XSS. Please use escape(https://${https_domain}) as a wrapper for escaping


570-571: ⚠️ Potential issue

Ensure jQuery is available or replace '$' with vanilla JavaScript

The usage of $ suggests reliance on jQuery, but $ may not be defined, leading to runtime errors. Verify that jQuery is included, or refactor the code to use vanilla JavaScript.

If jQuery is not included, import it:

+import $ from 'jquery';

Alternatively, replace $ with vanilla JavaScript:

-        $('double-bounce-spinner').remove();
-        $('#wrapper').removeClass('fade');
+        document.querySelector('double-bounce-spinner').remove();
+        document.getElementById('wrapper').classList.remove('fade');

Repeat the changes at lines 585-586.

Also applies to: 585-586

🧰 Tools
🪛 eslint

[error] 570-570: '$' is not defined.

(no-undef)


[error] 571-571: '$' is not defined.

(no-undef)


579-579: 🧹 Nitpick (assertive)

Avoid reassigning properties of function parameters

Modifying properties of injected parameters like $rootScope can lead to unexpected side effects. Consider using a service or a dedicated state management solution to handle global state.

Example:

-          $rootScope.noHttps = true;
+          appState.noHttps = true;

Create appState service to manage shared state across the application.

Also applies to: 583-583

🧰 Tools
🪛 eslint

[error] 579-579: Assignment to property of function parameter '$rootScope'.

(no-param-reassign)


373-373: 🧹 Nitpick (assertive)

Use 'const' for variables that are never reassigned

The variable response is never reassigned after its initial assignment. Using const improves code readability and prevents accidental reassignments.

Apply this diff:

-        let response = await fetch('/auth/who_am_i');
+        const response = await fetch('/auth/who_am_i');
📝 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 response = await fetch('/auth/who_am_i');
🧰 Tools
🪛 Biome (1.9.4)

[error] 373-373: This let declares a variable that is only assigned once.

'response' is never reassigned.

Safe fix: Use const instead.

(lint/style/useConst)

🪛 eslint

[error] 373-373: 'response' is never reassigned. Use 'const' instead.

(prefer-const)


366-366: ⚠️ Potential issue

Avoid empty catch blocks to prevent silent failures

The empty catch block suppresses errors, making debugging difficult. At a minimum, log the error to the console.

Apply this diff to handle the error:

       } catch (e) {
-       }
+         console.error('Error in preStart function:', e);
+       }
📝 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.

    } catch (e) {
         console.error('Error in preStart function:', e);
    }
🧰 Tools
🪛 eslint

[error] 366-366: Empty block statement.

(no-empty)

🪛 GitHub Check: Codacy Static Code Analysis

[warning] 366-366: app/scripts/app.js#L366
Empty block statement.


371-385: ⚠️ Potential issue

Avoid variable shadowing of 'rolesFactory'

The parameter rolesFactory in getUserType shadows a variable in the outer scope. This can lead to confusion and potential bugs. Remove the parameter to use the rolesFactory from the outer scope.

Apply this diff:

-async function getUserType(rolesFactory) {
+async function getUserType() {
    try {
        const response = await fetch('/auth/who_am_i');
        if (response.status === 200) {
            const user = await response.json();
            rolesFactory.setRole(user.user_type, false);
            return 'ok';
        }
        if (response.status === 401) {
            return 'login';
        }
    } catch (e) {}
    rolesFactory.setRole('user', true);
    return 'ok';
}
📝 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.

async function getUserType() {
  try {
    const response = await fetch('/auth/who_am_i');
    if (response.status === 200) {
      const user = await response.json();
      rolesFactory.setRole(user.user_type, false);
      return 'ok';
    }
    if (response.status === 401) {
      return 'login';
    }
  } catch (e) {}
  rolesFactory.setRole('user', true);
  return 'ok';
}
🧰 Tools
🪛 Biome (1.9.4)

[error] 373-373: This let declares a variable that is only assigned once.

'response' is never reassigned.

Safe fix: Use const instead.

(lint/style/useConst)

🪛 eslint

[error] 371-371: 'rolesFactory' is already declared in the upper scope on line 52 column 8.

(no-shadow)


[error] 373-373: 'response' is never reassigned. Use 'const' instead.

(prefer-const)


[error] 382-382: Empty block statement.

(no-empty)


356-362: 🧹 Nitpick (assertive)

Use camelCase for variable names

The variable https_domain does not follow the camelCase convention. Rename it to httpsDomain for consistency.

Apply this diff:

-          const https_domain = await response.text();
-          response = await fetch(`https://${https_domain}/https/check`, {
+          const httpsDomain = await response.text();
+          response = await fetch(`https://${httpsDomain}/https/check`, {
             method: 'GET',
             mode: 'cors',
           });
           if (response.status === 200) {
-            window.location.href = `https://${https_domain}`;
+            window.location.href = `https://${httpsDomain}`;
           }
📝 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 httpsDomain = await response.text();
        response = await fetch(`https://${httpsDomain}/https/check`, {
          method: 'GET',
          mode: 'cors',
        });
        if (response.status === 200) {
          window.location.href = `https://${httpsDomain}`;
🧰 Tools
🪛 eslint

[error] 356-356: Identifier 'https_domain' is not in camel case.

(camelcase)


[error] 357-357: Identifier 'https_domain' is not in camel case.

(camelcase)


[error] 362-362: Identifier 'https_domain' is not in camel case.

(camelcase)

🪛 GitHub Check: Codacy Static Code Analysis

[warning] 362-362: app/scripts/app.js#L362
Dangerous location.href assignment can lead to XSS. Please use escape(https://${https_domain}) as a wrapper for escaping

app/scripts/react-directives/components/access-level/accessLevelStore.js (1)

16-20: 🧹 Nitpick (assertive)

Handle potential errors in 'asyncCheckRights'

If asyncCheckRights fails, accessGranted remains false, but no error is communicated. Consider adding error handling to inform the user or take corrective action.

Example:

      setRole(value) {
        this.rolesFactory.asyncCheckRights(value, () => {
          runInAction(() => {
            this.accessGranted = true;
          });
+       }, (error) => {
+         // Handle error, e.g., log or update the state
+         console.error('Error checking rights:', error);
+       });
      }
📝 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.

    this.rolesFactory.asyncCheckRights(value, () => {
      runInAction(() => {
        this.accessGranted = true;
      });
    }, (error) => {
      // Handle error, e.g., log or update the state
      console.error('Error checking rights:', error);
    });
app/scripts/controllers/systemController.js (1)

7-12: 🧹 Nitpick (assertive)

Initialize 'haveRights' to 'false' before rights check

Initializing this.haveRights to false ensures that the controller accurately reflects the user's rights before the asynchronous check completes.

Apply this diff:

function SystemCtrl(rolesFactory) {
  'ngInject';

+ this.haveRights = false;
  rolesFactory.asyncCheckRights(rolesFactory.ROLE_THREE, () => {
    this.haveRights = true;
  });
  setReactLocale();
}

Committable suggestion skipped: line range outside the PR's diff.

app/scripts/controllers/networkConnectionsController.js (2)

13-14: ⚠️ Potential issue

Security: Improve path validation.

The current path validation is basic. Consider adding more robust path sanitization to prevent path traversal attacks.

-if (!/^\//.test($scope.file.schemaPath))
+const sanitizedPath = $scope.file.schemaPath.replace(/\.\./g, '').replace(/\/+/g, '/');
+if (!/^\//.test(sanitizedPath))
   $scope.file.schemaPath = '/' + $scope.file.schemaPath;

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 Biome (1.9.4)

[error] 14-14: Template literals are preferred over string concatenation.

Unsafe fix: Use a template literal.

(lint/style/useTemplate)

🪛 eslint

[error] 14-14: Expected no linebreak before this statement.

(nonblock-statement-body-position)


[error] 14-14: Expected { after 'if' condition.

(curly)


[error] 14-14: Unexpected string concatenation.

(prefer-template)


7-15: ⚠️ Potential issue

Critical: Add error handling for rights check failure.

The async rights check lacks error handling. If the check fails, the application state remains undefined.

Add error handling:

 rolesFactory.asyncCheckRights(rolesFactory.ROLE_THREE, () => {
   this.haveRights = true;
   $scope.file = {
     schemaPath: $stateParams.path,
   };

   if (!/^\//.test($scope.file.schemaPath))
     $scope.file.schemaPath = '/' + $scope.file.schemaPath;
+ }, (error) => {
+   this.haveRights = false;
+   console.error('Rights check failed:', error);
 });
📝 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.

    rolesFactory.asyncCheckRights(rolesFactory.ROLE_THREE, () => {
      this.haveRights = true;
      $scope.file = {
        schemaPath: $stateParams.path,
      };

      if (!/^\//.test($scope.file.schemaPath))
        $scope.file.schemaPath = '/' + $scope.file.schemaPath;
    }, (error) => {
      this.haveRights = false;
      console.error('Rights check failed:', error);
    });
🧰 Tools
🪛 Biome (1.9.4)

[error] 14-14: Template literals are preferred over string concatenation.

Unsafe fix: Use a template literal.

(lint/style/useTemplate)

🪛 eslint

[error] 14-14: Expected no linebreak before this statement.

(nonblock-statement-body-position)


[error] 14-14: Expected { after 'if' condition.

(curly)


[error] 14-14: Unexpected string concatenation.

(prefer-template)

app/scripts/react-directives/login/login-modal.js (3)

34-36: 🛠️ Refactor suggestion

Security: Enhance HTTPS warning behavior.

The HTTP warning implementation should be more strict for login operations.

Consider forcing HTTPS for login:

 scope.$watch('httpWarning', newValue => {
+  if (newValue && window.location.protocol !== 'https:') {
+    window.location.href = window.location.href.replace('http:', 'https:');
+    return;
+  }
   scope.store.setHttpWarning(newValue);
 });
📝 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.

      scope.$watch('httpWarning', newValue => {
        if (newValue && window.location.protocol !== 'https:') {
          window.location.href = window.location.href.replace('http:', 'https:');
          return;
        }
        scope.store.setHttpWarning(newValue);
      });
🧰 Tools
🪛 eslint

[error] 34-34: Expected parentheses around arrow function argument.

(arrow-parens)


20-22: 🧹 Nitpick (assertive)

Potential race condition in root unmounting.

Check for existing root before unmounting could lead to race conditions.

-if (scope.root) {
+if (scope.root && !scope.root._unmounted) {
   scope.root.unmount();
+  scope.root._unmounted = true;
 }

Committable suggestion skipped: line range outside the PR's diff.


24-26: ⚠️ Potential issue

Security: Protect sensitive login state.

The login store might contain sensitive information. Ensure proper cleanup of credentials.

Add credential cleanup in unmount:

 element.on('$destroy', () => {
+  scope.store.clearCredentials(); // Add this method to store
   scope.root.unmount();
 });

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 eslint

[error] 24-24: Assignment to property of function parameter 'scope'.

(no-param-reassign)


[error] 25-25: Assignment to property of function parameter 'scope'.

(no-param-reassign)

app/scripts/react-directives/login/loginModal.jsx (3)

23-25: ⚠️ Potential issue

Enhance error handling security.

The error messages could potentially leak sensitive information. Consider:

  1. Using generic error messages for failed login attempts
  2. Logging detailed errors server-side only
🧰 Tools
🪛 Biome (1.9.4)

[error] 23-23: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.

Unsafe fix: Use a SelfClosingElement instead

(lint/style/useSelfClosingElements)


[error] 25-25: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.

Unsafe fix: Use a SelfClosingElement instead

(lint/style/useSelfClosingElements)

🪛 eslint

[error] 23-23: Empty components are self-closing

(react/self-closing-comp)


[error] 25-25: Empty components are self-closing

(react/self-closing-comp)


23-23: 🧹 Nitpick (assertive)

Fix self-closing tags.

Per static analysis tools, empty JSX elements should be self-closing.

Apply this diff:

-        {store.httpWarning && <ErrorBar msg={t('login.errors.http-warning')}></ErrorBar>}
+        {store.httpWarning && <ErrorBar msg={t('login.errors.http-warning')} />}
-        {store.error && <ErrorBar msg={t('login.errors.failed')}></ErrorBar>}
+        {store.error && <ErrorBar msg={t('login.errors.failed')} />}

Also applies to: 25-25

🧰 Tools
🪛 Biome (1.9.4)

[error] 23-23: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.

Unsafe fix: Use a SelfClosingElement instead

(lint/style/useSelfClosingElements)

🪛 eslint

[error] 23-23: Empty components are self-closing

(react/self-closing-comp)


38-40: 🛠️ Refactor suggestion

Add PropTypes validation for store prop.

The store prop lacks type validation, which could lead to runtime errors.

Add the following after the component:

CreateLoginModal.propTypes = {
  store: PropTypes.shape({
    active: PropTypes.bool.isRequired,
    formStore: PropTypes.object.isRequired,
    httpWarning: PropTypes.bool,
    error: PropTypes.bool,
    postLogin: PropTypes.func.isRequired
  }).isRequired
};
🧰 Tools
🪛 eslint

[error] 38-38: 'store' is missing in props validation

(react/prop-types)

app/scripts/controllers/scriptsController.js (1)

50-50: 🛠️ Refactor suggestion

Fix undefined 'angular' reference.

The angular global is flagged as undefined. Import Angular properly to ensure compatibility with strict mode and bundlers.

Add import statement at the top of the file:

+import angular from 'angular';

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 eslint

[error] 50-50: 'angular' is not defined.

(no-undef)

🪛 GitHub Check: Codacy Static Code Analysis

[failure] 50-50: app/scripts/controllers/scriptsController.js#L50
'angular' is not defined.

app/scripts/services/roles.factory.js (3)

62-65: ⚠️ Potential issue

Fix object injection vulnerability.

The typeToRoleId lookup using user input is vulnerable to prototype pollution attacks.

Apply this diff to secure the role mapping:

   const typeToRoleId = {
     admin: roles.ROLE_THREE,
     operator: roles.ROLE_TWO,
     user: roles.ROLE_ONE,
   };
+  Object.freeze(typeToRoleId);

   roles.setRole = (user_type, notConfiguredAdmin) => {
     this.roleIsSet = true;
-    const roleId = typeToRoleId[user_type] || DEFAULT_ROLE;
+    const roleId = Object.prototype.hasOwnProperty.call(typeToRoleId, user_type) 
+      ? typeToRoleId[user_type] 
+      : DEFAULT_ROLE;

Also applies to: 70-70


32-34: 🛠️ Refactor suggestion

Protect role state from manipulation.

The role state is mutable and could be manipulated by malicious code.

Consider using private fields or Object.freeze to protect the role state:

-  roles.current = { role: DEFAULT_ROLE, roles: roles.ROLES[DEFAULT_ROLE - 1] };
+  Object.defineProperty(roles, 'current', {
+    value: Object.freeze({ 
+      role: DEFAULT_ROLE, 
+      roles: Object.freeze(roles.ROLES[DEFAULT_ROLE - 1]) 
+    }),
+    writable: false,
+    configurable: false
+  });
📝 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.

  Object.defineProperty(roles, 'current', {
    value: Object.freeze({ 
      role: DEFAULT_ROLE, 
      roles: Object.freeze(roles.ROLES[DEFAULT_ROLE - 1]) 
    }),
    writable: false,
    configurable: false
  });
  roles.notConfiguredAdmin = false;
  roles.roleIsSet = false;

68-73: 🛠️ Refactor suggestion

Add input validation for setRole function.

The setRole function accepts user input without validation.

Add input validation:

-  roles.setRole = (user_type, notConfiguredAdmin) => {
+  roles.setRole = (userType, notConfiguredAdmin) => {
+    if (typeof userType !== 'string') {
+      throw new TypeError('userType must be a string');
+    }
     this.roleIsSet = true;
-    const roleId = typeToRoleId[user_type] || DEFAULT_ROLE;
+    const roleId = Object.prototype.hasOwnProperty.call(typeToRoleId, userType)
+      ? typeToRoleId[userType]
+      : DEFAULT_ROLE;
     roles.current = { role: roleId, roles: roles.ROLES[roleId - 1] };
     roles.notConfiguredAdmin = !!notConfiguredAdmin;
     roles.whenReadyResolve();
   };
📝 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.

  roles.setRole = (userType, notConfiguredAdmin) => {
    if (typeof userType !== 'string') {
      throw new TypeError('userType must be a string');
    }
    this.roleIsSet = true;
    const roleId = Object.prototype.hasOwnProperty.call(typeToRoleId, userType)
      ? typeToRoleId[userType]
      : DEFAULT_ROLE;
    roles.current = { role: roleId, roles: roles.ROLES[roleId - 1] };
    roles.notConfiguredAdmin = !!notConfiguredAdmin;
    roles.whenReadyResolve();
  };
🧰 Tools
🪛 eslint

[error] 68-68: Identifier 'user_type' is not in camel case.

(camelcase)


[error] 70-70: Identifier 'user_type' is not in camel case.

(camelcase)

🪛 GitHub Check: Codacy Static Code Analysis

[warning] 70-70: app/scripts/services/roles.factory.js#L70
Generic Object Injection Sink

app/scripts/react-directives/login/store.js (3)

77-77: 🧹 Nitpick (assertive)

Maintain consistent arrow function style.

Add parentheses around single parameters in arrow functions for consistency.

-    Object.values(this.formStore.params).forEach(param => param.setReadOnly(true));
+    Object.values(this.formStore.params).forEach((param) => param.setReadOnly(true));

-    }).then(response => {
+    }).then((response) => {

-        response.json().then(data => {
+        response.json().then((data) => {

-        Object.values(this.formStore.params).forEach(param => param.setReadOnly(false));
+        Object.values(this.formStore.params).forEach((param) => param.setReadOnly(false));

Also applies to: 84-84, 86-86, 92-92

🧰 Tools
🪛 eslint

[error] 77-77: Expected parentheses around arrow function argument.

(arrow-parens)


90-94: 🛠️ Refactor suggestion

Improve error handling to prevent information leakage.

The error handling should not expose whether the failure was due to invalid credentials or other server issues.

     } else {
-      this.setError();
+      if (response.status === 401) {
+        this.setError();
+      } else {
+        // Log the error internally but show generic message to user
+        console.error('Login failed:', response.status);
+        this.setError();
+      }
       Object.values(this.formStore.params).forEach(param => param.setReadOnly(false));
     }
📝 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.

      } else {
        if (response.status === 401) {
          this.setError();
        } else {
          // Log the error internally but show generic message to user
          console.error('Login failed:', response.status);
          this.setError();
        }
        Object.values(this.formStore.params).forEach(param => param.setReadOnly(false));
      }
    });
🧰 Tools
🪛 eslint

[error] 92-92: Expected parentheses around arrow function argument.

(arrow-parens)


78-84: ⚠️ Potential issue

Add CSRF protection to login request.

The login endpoint needs protection against Cross-Site Request Forgery (CSRF) attacks.

Add the CSRF token to the request headers:

 fetch('/login', {
   method: 'POST',
   headers: {
     'Content-Type': 'application/json',
+    'X-CSRF-Token': document.querySelector('meta[name="csrf-token"]').content,
   },
   body: JSON.stringify(this.formStore.value),
📝 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.

    fetch('/login', {
      method: 'POST',
      headers: {
        'Content-Type': 'application/json',
        'X-CSRF-Token': document.querySelector('meta[name="csrf-token"]').content,
      },
      body: JSON.stringify(this.formStore.value),
    }).then(response => {
🧰 Tools
🪛 eslint

[error] 84-84: Expected parentheses around arrow function argument.

(arrow-parens)

app/scripts/react-directives/users/usersPage.jsx (2)

34-37: 🧹 Nitpick (assertive)

Address JSX style and accessibility issues.

Several style and accessibility improvements are needed:

-              <th>{t('users.labels.login')}</th>
-              <th>{t('users.labels.type')}</th>
-              <th></th>
+              <th scope="col">{t('users.labels.login')}</th>
+              <th scope="col">{t('users.labels.type')}</th>
+              <th scope="col" aria-label={t('users.labels.actions')}></th>

-            {store.users.map(user => (
+            {store.users.map((user) => (

-                <td style={{ verticalAlign: 'middle' }}>{t('users.labels.' + user.type)}</td>
+                <td style={{ verticalAlign: 'middle' }}>{t(`users.labels.${user.type}`)}</td>

Also applies to: 40-40, 43-43

🧰 Tools
🪛 Biome (1.9.4)

[error] 36-36: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.

Unsafe fix: Use a SelfClosingElement instead

(lint/style/useSelfClosingElements)

🪛 eslint

[error] 36-36: A control must be associated with a text label.

(jsx-a11y/control-has-associated-label)


[error] 36-36: Empty components are self-closing

(react/self-closing-comp)


52-57: ⚠️ Potential issue

Add access control check for delete operation.

The delete button should only be visible to users with appropriate permissions.

     <Button
       type="danger"
       icon="glyphicon glyphicon-trash"
+      disabled={!store.hasDeletePermission(user)}
       onClick={() => store.deleteUser(user)}
     />

Committable suggestion skipped: line range outside the PR's diff.

app/scripts/controllers/navigationController.js (3)

11-17: ⚠️ Potential issue

Avoid storing sensitive information in $rootScope.

Storing role information in $rootScope poses security risks and violates Angular best practices:

  1. Information could be accessible from unexpected places
  2. Makes state management harder to track
  3. $rootScope usage is discouraged

Consider using a dedicated AuthService instead:

class AuthService {
  constructor(rolesFactory) {
    this.roles = rolesFactory;
  }
  
  getRoles() {
    return this.roles;
  }
}
🧰 Tools
🪛 eslint

[error] 12-13: Missing trailing comma.

(comma-dangle)


[error] 17-17: Assignment to property of function parameter '$rootScope'.

(no-param-reassign)


90-92: 🧹 Nitpick (assertive)

Strengthen access control check.

The current implementation could be more robust by:

  1. Adding explicit null checks
  2. Handling edge cases
     $scope.showAccessControl = function () {
-      return rolesFactory.current.roles.isAdmin || rolesFactory.notConfiguredAdmin;
+      return rolesFactory?.current?.roles?.isAdmin === true || rolesFactory?.notConfiguredAdmin === true;
     };
📝 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.

    $scope.showAccessControl = function () {
      return rolesFactory?.current?.roles?.isAdmin === true || rolesFactory?.notConfiguredAdmin === true;
    };

94-100: ⚠️ Potential issue

Improve logout security.

The logout implementation has several security issues:

  1. Missing CSRF protection
  2. Using location.reload() is unsafe as it might preserve sensitive data
     $scope.logout = function () {
       fetch('/logout', {
         method: 'POST',
+        headers: {
+          'X-CSRF-Token': document.querySelector('meta[name="csrf-token"]').content,
+        },
       }).then(() => {
-        location.reload();
+        window.location.href = '/login';  // Redirect to login page instead
       });
     };
📝 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.

    $scope.logout = function () {
      fetch('/logout', {
        method: 'POST',
        headers: {
          'X-CSRF-Token': document.querySelector('meta[name="csrf-token"]').content,
        },
      }).then(() => {
        window.location.href = '/login';  // Redirect to login page instead
      });
    };
🧰 Tools
🪛 eslint

[error] 98-98: Unexpected use of 'location'.

(no-restricted-globals)

webpack.config.js (2)

11-11: 🧹 Nitpick (assertive)

Consider using ES modules import syntax.

Replace CommonJS require with ES modules import for consistency with modern JavaScript practices.

-const dotenv = require('dotenv');
+import dotenv from 'dotenv';
📝 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.

import dotenv from 'dotenv';
🧰 Tools
🪛 GitHub Check: Codacy Static Code Analysis

[failure] 11-11: webpack.config.js#L11
Require statement not part of import statement.


50-52: 💡 Codebase verification

Add error handling for missing .env file

The current implementation will inject "undefined" as the value of process.env when .env file is missing, which can cause runtime errors. Consider updating the DefinePlugin configuration with a fallback:

new webpack.DefinePlugin({
  'process.env': JSON.stringify(dotenv.config().parsed || {}),
}),
  • webpack.config.js: Add error handling and consider creating an .env.example file to document required environment variables.
🔗 Analysis chain

Verify error handling for missing .env file.

The current implementation might throw an error if the .env file is missing. Consider adding a fallback for production environments.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if .env file exists and verify dotenv error handling
if [ ! -f ".env" ]; then
  echo "Warning: .env file not found"
  # Test webpack build without .env
  NODE_ENV=production webpack --config webpack.config.js
fi

Length of output: 245


Script:

#!/bin/bash
# Check webpack config content and search for environment-related patterns
echo "=== Webpack Config Content ==="
cat webpack.config.js

echo -e "\n=== Searching for Environment Files ==="
fd -e env

echo -e "\n=== Searching for Environment Variable Usage ==="
rg "process\.env" -A 2 -B 2

Length of output: 10018

app/index.ejs (2)

28-35: 🧹 Nitpick (assertive)

Consider consolidating alert messages.

Multiple alert divs with similar structure could be consolidated into a reusable component.

Consider creating a shared alert component:

-<div class="alert alert-danger" role="alert" ng-cloak ng-if="noHttps">
-  <span class="glyphicon glyphicon-exclamation-sign" aria-hidden="true"></span>
-  <span translate>{{'app.errors.no-https'}}</span>
-</div>
-<div class="alert alert-danger" role="alert" ng-cloak ng-if="roles.notConfiguredAdmin">
-  <span class="glyphicon glyphicon-exclamation-sign" aria-hidden="true"></span>
-  <span translate>{{'app.errors.not-configured-admin'}}</span>
-</div>
+<alert-message type="danger" show="noHttps" message="app.errors.no-https"></alert-message>
+<alert-message type="danger" show="roles.notConfiguredAdmin" message="app.errors.not-configured-admin"></alert-message>

Committable suggestion skipped: line range outside the PR's diff.


53-65: 🧹 Nitpick (assertive)

Verify ARIA attributes for accessibility.

The user menu dropdown implementation needs additional ARIA attributes for better accessibility.

-<div class="dropdown user-menu navbar-right" ng-if="!roles.notConfiguredAdmin">
+<div class="dropdown user-menu navbar-right" ng-if="!roles.notConfiguredAdmin" role="navigation">
   <i class="glyphicon glyphicon-user" id="userMenu" type="button" data-toggle="dropdown" 
      aria-label="User menu" aria-haspopup="true" aria-expanded="false"></i>
-  <ul class="dropdown-menu" aria-labelledby="userMenu">
+  <ul class="dropdown-menu" aria-labelledby="userMenu" role="menu">
     <li>
-      <span class='user-name' translate>{{roles.current.roles.name}}</span>
+      <span class='user-name' translate role="menuitem">{{roles.current.roles.name}}</span>
     </li>
     <li role="separator" class="divider"></li>
     <li>
-      <a href="#" ng-click="logout()" translate>{{'app.buttons.logout'}}</a>
+      <a href="#" ng-click="logout()" translate role="menuitem">{{'app.buttons.logout'}}</a>
     </li>
   </ul>
 </div>
📝 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.

                <div class="dropdown user-menu navbar-right" ng-if="!roles.notConfiguredAdmin" role="navigation">
                    <i class="glyphicon glyphicon-user" id="userMenu" type="button" data-toggle="dropdown" aria-label="User menu" aria-haspopup="true" aria-expanded="false"></i>
                    <ul class="dropdown-menu" aria-labelledby="userMenu" role="menu">
                        <li>
                            <span class='user-name' translate role="menuitem">{{roles.current.roles.name}}</span>
                        </li>
                        <li role="separator" class="divider"></li>
                        <li>
                            <a href="#" ng-click="logout()" translate role="menuitem">{{'app.buttons.logout'}}</a>
                        </li>
                    </ul>
                </div>
              </div>
app/scripts/controllers/logsController.js (2)

4-14: 🧹 Nitpick (assertive)

Add trailing comma for better git diffs.

Add a trailing comma to the last constructor parameter for better git diffs when adding new parameters.

 constructor(
   $scope,
   $injector,
   $q,
   $uibModal,
   $element,
   $translate,
   $rootScope,
   $filter,
-  rolesFactory
+  rolesFactory,
 ) {
📝 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.

  constructor(
    $scope,
    $injector,
    $q,
    $uibModal,
    $element,
    $translate,
    $rootScope,
    $filter,
    rolesFactory,
  ) {
🧰 Tools
🪛 eslint

[error] 13-14: Missing trailing comma.

(comma-dangle)


114-131: 🛠️ Refactor suggestion

Improve error handling in async rights check.

The error handling in the async rights check could be improved:

  1. Arrow functions should have parentheses around parameters
  2. Assignment in return statement should be avoided
  3. Variable shadowing should be fixed
 rolesFactory.asyncCheckRights(rolesFactory.ROLE_TWO, () => {
   this.haveRights = true;
   whenMqttReady()
     .then(() => this.LogsProxy.hasMethod('Load'))
     .then(result => {
       if (result) {
-        this.LogsProxy.hasMethod('CancelLoad').then(result => (this.canCancelLoad = result));
+        this.LogsProxy.hasMethod('CancelLoad').then((hasMethod) => {
+          this.canCancelLoad = hasMethod;
+        });
         vm.loadBootsAndServices();
       } else {
         this.enableSpinner = false;
         this.errors.catch('log.labels.unavailable')();
       }
     })
-    .catch(err => {
+    .catch((err) => {
       this.enableSpinner = false;
       this.errors.catch('logs.labels.unavailable')(err);
     });
 });
📝 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.

    rolesFactory.asyncCheckRights(rolesFactory.ROLE_TWO, () => {
      this.haveRights = true;
      whenMqttReady()
        .then(() => this.LogsProxy.hasMethod('Load'))
        .then(result => {
          if (result) {
            this.LogsProxy.hasMethod('CancelLoad').then((hasMethod) => {
              this.canCancelLoad = hasMethod;
            });
            vm.loadBootsAndServices();
          } else {
            this.enableSpinner = false;
            this.errors.catch('log.labels.unavailable')();
          }
        })
        .catch((err) => {
          this.enableSpinner = false;
          this.errors.catch('logs.labels.unavailable')(err);
        });
    });
🧰 Tools
🪛 Biome (1.9.4)

[error] 120-120: The assignment should not be in an expression.

The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.

(lint/suspicious/noAssignInExpressions)

🪛 eslint

[error] 118-118: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 120-120: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 120-120: Arrow function should not return assignment.

(no-return-assign)


[error] 120-120: 'result' is already declared in the upper scope on line 118 column 15.

(no-shadow)


[error] 127-127: Expected parentheses around arrow function argument.

(arrow-parens)

app/views/network-connections.html (1)

2-2: 🧹 Nitpick (assertive)

LGTM! Consider adding aria-label for better accessibility.

The access control implementation looks good, but we could improve accessibility for screen readers.

-ng-hide="$ctrl.haveRights">{{'configurations.labels.access-notice' | translate}}
+ng-hide="$ctrl.haveRights">
+  <span aria-label="Access Notice">{{'configurations.labels.access-notice' | translate}}</span>
📝 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.

ng-hide="$ctrl.haveRights">
  <span aria-label="Access Notice">{{'configurations.labels.access-notice' | translate}}</span>
README.md (1)

12-12: 🧹 Nitpick (assertive)

Enhance the documentation clarity and fix grammar.

While the addition of MQTT broker configuration instructions is valuable, consider these improvements:

  1. Add a comma after "url"
  2. Provide the expected format of the URL
  3. Add an example

Apply this diff:

-To change the mqtt broker url create a "MQTT_BROKER_URI" variable in the .env file with your url.
+To change the mqtt broker url, create a "MQTT_BROKER_URI" variable in the .env file with your url. For example:
+```
+MQTT_BROKER_URI=mqtt://localhost:1883
+```
📝 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.

To change the mqtt broker url, create a "MQTT_BROKER_URI" variable in the .env file with your url. For example:
🧰 Tools
🪛 LanguageTool

[uncategorized] ~12-~12: A comma might be missing here.
Context: ... for preview. To change the mqtt broker url create a "MQTT_BROKER_URI" variable in ...

(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)

app/views/devices.html (1)

1-3: 💡 Codebase verification

⚠️ Potential issue

Critical security measures missing in device management interface

The following security issues were identified:

  • No CSRF protection for device deletion operations
  • Insufficient input sanitization (escapeValue disabled in translations)
  • Missing security headers (CSP, X-Frame-Options, X-XSS-Protection)
🔗 Analysis chain

Implement additional security measures for device management.

The device management interface requires enhanced security:

  1. Add CSRF protection for device deletion
  2. Sanitize device names to prevent XSS
  3. Validate locale parameter

Let's verify the security measures:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for security measures in device management
echo "=== XSS Protection ==="
rg -A 5 "sanitize|escape" app/

echo "=== CSRF Protection ==="
rg -A 5 "csrf|xsrf" app/

Length of output: 75556


Script:

#!/bin/bash
# Search for device deletion and validation logic
echo "=== Device Deletion Endpoints ==="
rg -A 5 "deleteDevice|removeDevice" app/

echo "=== Device Name Validation ==="
rg -A 5 "deviceName|name.*validation" app/

echo "=== Locale/Translation Handling ==="
rg -A 5 "locale.*param|i18n.*locale" app/

Length of output: 67392


Script:

#!/bin/bash
# Search for device API endpoints and form handling
echo "=== Device API Endpoints ==="
rg -A 5 "api.*device|device.*api" app/

echo "=== Form Submissions ==="
rg -A 5 "form.*submit|ng-submit|onSubmit" app/

echo "=== Translation Usage ==="
rg -A 5 "translate.*filter|i18n.*translate" app/

Length of output: 66016


Script:

#!/bin/bash
# Search for security measures
echo "=== CSRF Token Usage ==="
rg -A 5 "csrf|xsrf|X-CSRF" app/scripts/

echo "=== Security Headers ==="
rg -A 5 "Content-Security-Policy|X-Frame-Options|X-XSS-Protection" app/

echo "=== Input Sanitization ==="
rg -A 5 "sanitize|escape|encodeURI" app/scripts/

Length of output: 8294

app/scripts/controllers/configController.js (2)

70-73: 🛠️ Refactor suggestion

Initialize haveRights property.

The this.haveRights property should be initialized to false at the start of the constructor to prevent undefined behavior during the async check.

Add initialization:

 constructor(...) {
+  this.haveRights = false;
   // ... existing code ...
   rolesFactory.asyncCheckRights(...)
 }

Committable suggestion skipped: line range outside the PR's diff.


70-73: ⚠️ Potential issue

Add error handling for failed rights checks.

The asynchronous rights check lacks error handling. If the rights check fails, there's no feedback to the user or error recovery mechanism.

Consider adding error handling:

 rolesFactory.asyncCheckRights(rolesFactory.ROLE_THREE, () => {
   this.haveRights = true;
   whenMqttReady().then(load);
+}, (error) => {
+  this.haveRights = false;
+  errors.catch('configurations.errors.access')(error);
 });

Committable suggestion skipped: line range outside the PR's diff.

Makefile (1)

47-47: ⚠️ Potential issue

Review directory permissions for security.

While the new fonts directory correctly uses 0755 permissions, other web directories are set to 0777 which is overly permissive and poses a security risk.

Consider updating permissions for all web directories:

-install -d -m 0777 $(DESTDIR)/var/www/css
-install -d -m 0777 $(DESTDIR)/var/www/images
-install -d -m 0777 $(DESTDIR)/var/www/uploads
-install -d -m 0777 $(DESTDIR)/var/www/scripts/i18n
+install -d -m 0755 $(DESTDIR)/var/www/css
+install -d -m 0755 $(DESTDIR)/var/www/images
+install -d -m 0750 $(DESTDIR)/var/www/uploads
+install -d -m 0755 $(DESTDIR)/var/www/scripts/i18n
 install -d -m 0755 $(DESTDIR)/var/www/fonts

Note: The uploads directory might need more restrictive permissions (0750) since it handles user-uploaded content.

Also applies to: 56-56

app/scripts/i18n/app/en.json (1)

8-9: 🧹 Nitpick (assertive)

LGTM! Consider enhancing security messages.

The security-related error messages are clear and informative. However, consider adding more specific guidance in the HTTPS message.

Consider enhancing the HTTPS warning message:

-"no-https": "The Web UI is running over HTTP. It is recommended to use HTTPS to ensure the security of your credentials and data"
+"no-https": "The Web UI is running over HTTP. It is recommended to use HTTPS to ensure the security of your credentials and data. Contact your system administrator to enable HTTPS or visit our documentation at <docs_url> for setup instructions"

Committable suggestion skipped: line range outside the PR's diff.

app/styles/css/new.css (1)

384-392: 🧹 Nitpick (assertive)

Consider adding transition for hover opacity.

The user icon has a color transition but could benefit from an opacity transition for smoother hover effects.

 .user-menu .glyphicon-user {
   font-size: 20px;
   color: lightgray;
-  transition: color 0.2s ease;
+  transition: color 0.2s ease, opacity 0.2s ease;
+  opacity: 0.8;
 }

 .user-menu .glyphicon-user:hover {
   color: gray;
+  opacity: 1;
 }
📝 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.

.user-menu .glyphicon-user {
  font-size: 20px;
  color: lightgray;
  transition: color 0.2s ease, opacity 0.2s ease;
  opacity: 0.8;
}

.user-menu .glyphicon-user:hover {
  color: gray;
  opacity: 1;
}
app/styles/main.css (1)

184-186: 🧹 Nitpick (assertive)

Consider adding aria-label for accessibility.

The user icon button should have an aria-label for screen readers.

Add the following HTML attribute to the user icon element:

aria-label="User menu"
app/scripts/i18n/react/locales/ru/translations.json (1)

332-332: ⚠️ Potential issue

Avoid exposing raw error messages.

The "unknown" error message template could potentially leak sensitive implementation details through the {{msg}} parameter. Consider using a generic error message or sanitizing the error details.

-         "unknown": "Ошибка: {{msg}}"
+         "unknown": "Произошла неизвестная ошибка. Пожалуйста, обратитесь к администратору"
📝 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.

         "unknown": "Произошла неизвестная ошибка. Пожалуйста, обратитесь к администратору"

let response = await fetch('/https/redirect');
if (response.status === 200) {
const https_domain = await response.text();
response = await fetch(`https://${https_domain}/https/check`, {
Copy link
Member

Choose a reason for hiding this comment

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

мне кажется, что тут обязательно проверить, что это тот же контроллер. А то может это другой ответил, или captive portal какой-нибудь, или просто клиент пробросил 80 порт, а на 443 у него какая-нибудь админка роутера.

Я к тому, что этот check должен что-то возвращать, что подтверждает, что:

  1. это WB
  2. этот тот же самый WB

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Такое точно возможно?
Если ответит другой, то запрос не пройдёт из-за другого домена в сертификате.

'/auth/who_am_i',
'/logout',
],
target: process.env.MQTT_BROKER_URI || 'http://10.200.200.1',
Copy link
Member

Choose a reason for hiding this comment

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

а что это значит и зачем тут 10.200.200.1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Это для локальной разработки через npm. Мы подключаемся к реальному контроллеру, домен другой, браузер не шлёт куки. Тут мы проксируем веб сокет, чтоб браузер слал наши куки. Можно ещё настройками браузера это разрулить, но решили с Виктором, что так универсальнее

Copy link

@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: 16

♻️ Duplicate comments (1)
app/scripts/app.js (1)

357-357: ⚠️ Potential issue

Ensure Redirection is to the Same Controller to Prevent Security Risks

As previously noted, it's essential to verify that the response is from the same controller to prevent redirection to unintended destinations. There is a risk that the response could come from a different device, a captive portal, or an unauthorized source. The check should confirm that:

  1. This is a Wiren Board (WB) device.
  2. This is the same WB device initiating the request.
🧰 Tools
🪛 eslint

[error] 357-357: Identifier 'https_domain' is not in camel case.

(camelcase)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4613601 and a51203e.

📒 Files selected for processing (3)
  • app/scripts/app.js (6 hunks)
  • app/scripts/services/roles.factory.js (2 hunks)
  • webpack.config.js (5 hunks)
🧰 Additional context used
🪛 eslint
webpack.config.js

[error] 1-1: Definition for rule '@typescript-eslint/no-var-requires' was not found.

(@typescript-eslint/no-var-requires)

app/scripts/services/roles.factory.js

[error] 11-11: Unexpected dangling '_' in '_ROLE_ONE'.

(no-underscore-dangle)


[error] 16-16: Unexpected dangling '_' in '_ROLE_TWO'.

(no-underscore-dangle)


[error] 21-21: Unexpected dangling '_' in '_ROLE_THREE'.

(no-underscore-dangle)


[error] 37-37: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 44-44: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 44-46: Unexpected block statement surrounding arrow body; move the returned value immediately after the =>.

(arrow-body-style)


[error] 68-68: Identifier 'user_type' is not in camel case.

(camelcase)

app/scripts/app.js

[error] 96-96: Cannot use import declarations in modules that export using CommonJS (module.exports = 'foo' or exports.bar = 'hi')

(import/no-import-module-exports)


[error] 97-97: Cannot use import declarations in modules that export using CommonJS (module.exports = 'foo' or exports.bar = 'hi')

(import/no-import-module-exports)


[error] 356-356: Identifier 'https_domain' is not in camel case.

(camelcase)


[error] 357-357: Identifier 'https_domain' is not in camel case.

(camelcase)


[error] 362-362: Identifier 'https_domain' is not in camel case.

(camelcase)


[error] 373-373: 'rolesFactory' is already declared in the upper scope on line 52 column 8.

(no-shadow)


[error] 375-375: 'response' is never reassigned. Use 'const' instead.

(prefer-const)


[error] 384-384: Empty block statement.

(no-empty)


[error] 423-423: 'rolesFactory' is already declared in the upper scope on line 52 column 8.

(no-shadow)


[error] 572-572: '$' is not defined.

(no-undef)


[error] 573-573: '$' is not defined.

(no-undef)


[error] 576-576: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 581-581: Assignment to property of function parameter '$rootScope'.

(no-param-reassign)


[error] 583-583: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 583-583: 'res' is already declared in the upper scope on line 576 column 23.

(no-shadow)


[error] 585-585: Assignment to property of function parameter '$rootScope'.

(no-param-reassign)


[error] 587-587: '$' is not defined.

(no-undef)


[error] 588-588: '$' is not defined.

(no-undef)

🪛 Biome (1.9.4)
app/scripts/app.js

[error] 375-375: This let declares a variable that is only assigned once.

'response' is never reassigned.

Safe fix: Use const instead.

(lint/style/useConst)

🪛 GitHub Check: Codacy Static Code Analysis
app/scripts/app.js

[warning] 362-362: app/scripts/app.js#L362
Dangerous location.href assignment can lead to XSS. Please use escape(encodeURI(https://${https_domain})) as a wrapper for escaping


[warning] 384-384: app/scripts/app.js#L384
Empty block statement.

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (2)
webpack.config.js (2)

96-108: LGTM! Good separation of asset handling rules.

The split between SVG and font file handling improves maintainability, and the font output path is well-structured.


320-333: LGTM! Proxy configuration aligns with authentication requirements.

The proxy configuration correctly handles authentication endpoints and WebSocket connections. The fallback IP is intended for local development as discussed in previous comments.

Consider adding these security-related configurations to the proxy:

       {
         context: [
           '/auth/check_config',
           '/auth/users',
           '/login',
           '/mqtt',
           '/auth/who_am_i',
           '/logout',
         ],
         target: process.env.MQTT_BROKER_URI || 'http://10.200.200.1',
         ws: true,
+        secure: true,
+        changeOrigin: true,
+        headers: {
+          'X-Forwarded-Proto': 'https'
+        }
       },

webpack.config.js Show resolved Hide resolved
app/scripts/services/roles.factory.js Show resolved Hide resolved
app/scripts/services/roles.factory.js Show resolved Hide resolved
app/scripts/services/roles.factory.js Outdated Show resolved Hide resolved
app/scripts/services/roles.factory.js Outdated Show resolved Hide resolved
app/scripts/app.js Outdated Show resolved Hide resolved
Comment on lines 587 to 588
$('double-bounce-spinner').remove();
$('#wrapper').removeClass('fade');
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure '$' Is Defined or Replace With AngularJS Equivalent

The use of $ at lines 587-588 may cause runtime errors if jQuery is not included or $ is not defined. Consistent with AngularJS best practices, please replace $ with angular.element, or ensure that jQuery is properly integrated into the project.

Apply this change:

-               $('double-bounce-spinner').remove();
-               $('#wrapper').removeClass('fade');
+               angular.element('double-bounce-spinner').remove();
+               angular.element('#wrapper').removeClass('fade');

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 eslint

[error] 587-587: '$' is not defined.

(no-undef)


[error] 588-588: '$' is not defined.

(no-undef)

app/scripts/app.js Outdated Show resolved Hide resolved
app/scripts/app.js Outdated Show resolved Hide resolved
app/scripts/app.js Outdated Show resolved Hide resolved
Copy link

@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: 7

♻️ Duplicate comments (8)
app/scripts/app.js (8)

96-97: ⚠️ Potential issue

Ensure Consistent Module Import and Export Syntax

The import statements on lines 96-97 use ES6 import syntax, but the module exports use CommonJS module.exports. Mixing ES6 modules and CommonJS can lead to unexpected behavior or runtime errors. To ensure consistency and compatibility, consider refactoring the module to use a single module system throughout, preferably ES6 modules.

Apply this change to switch to CommonJS require syntax:

-import usersPageDirective from './react-directives/users/users';
-import loginModalDirective from './react-directives/login/login-modal';
+const usersPageDirective = require('./react-directives/users/users');
+const loginModalDirective = require('./react-directives/login/login-modal');

Alternatively, convert the module exports to ES6 export syntax to use ES6 modules consistently.

🧰 Tools
🪛 eslint

[error] 96-96: Cannot use import declarations in modules that export using CommonJS (module.exports = 'foo' or exports.bar = 'hi')

(import/no-import-module-exports)


[error] 97-97: Cannot use import declarations in modules that export using CommonJS (module.exports = 'foo' or exports.bar = 'hi')

(import/no-import-module-exports)


356-362: ⚠️ Potential issue

Address Potential XSS Risk and Conform to Variable Naming Conventions

  • Naming Convention: The variable https_domain does not follow camelCase naming conventions. Please rename it to httpsDomain to comply with the project's style guidelines.

  • Security Concern: Assigning window.location.href using data that could be influenced by an external source poses a potential XSS vulnerability. Even though https_domain is fetched from a server endpoint, it's crucial to validate and sanitize this value before using it in a redirect. Ensure that httpsDomain contains only trusted and expected values.

Apply the following changes:

         try {
-          let response = await fetch('/https/redirect');
+          const response = await fetch('/https/redirect');
           if (response.status === 200) {
-            const https_domain = await response.text();
-            response = await fetch(`https://${https_domain}/https/check`, {
+            const httpsDomain = await response.text();
+            response = await fetch(`https://${httpsDomain}/https/check`, {
               method: 'GET',
               mode: 'cors',
            });
             if (response.status === 200) {
-              window.location.href = encodeURI(`https://${https_domain}`);
+              // Validate that httpsDomain is a trusted domain before redirection
+              window.location.href = encodeURI(`https://${httpsDomain}`);
               return 'redirected';
             }
           }
🧰 Tools
🪛 eslint

[error] 356-356: Identifier 'https_domain' is not in camel case.

(camelcase)


[error] 357-357: Identifier 'https_domain' is not in camel case.

(camelcase)


[error] 362-362: Identifier 'https_domain' is not in camel case.

(camelcase)

🪛 GitHub Check: Codacy Static Code Analysis

[warning] 362-362: app/scripts/app.js#L362
Dangerous location.href assignment can lead to XSS. Please use escape(encodeURI(https://${https_domain})) as a wrapper for escaping


373-373: 🛠️ Refactor suggestion

Avoid Variable Shadowing by Renaming Parameters

The parameter rolesFactory in the function getUserType at line 373 shadows a variable declared in the upper scope. This can lead to confusion and unintended behavior. Please rename the parameter to prevent shadowing.

Apply this change:

-async function getUserType(rolesFactory) {
+async function getUserType(rolesService) {

And update the function calls accordingly:

-getUserType(rolesFactory).then(res => {
+getUserType(rolesFactory).then(result => {
🧰 Tools
🪛 eslint

[error] 373-373: 'rolesFactory' is already declared in the upper scope on line 52 column 8.

(no-shadow)


375-375: 🧹 Nitpick (assertive)

Use const Instead of let for Variables That Are Not Reassigned

The variable response is declared with let but is not reassigned after initialization. Using const enhances code readability and enforces immutability where applicable.

Apply this change:

-    let response = await fetch('/auth/who_am_i');
+    const response = await fetch('/auth/who_am_i');
🧰 Tools
🪛 Biome (1.9.4)

[error] 375-375: This let declares a variable that is only assigned once.

'response' is never reassigned.

Safe fix: Use const instead.

(lint/style/useConst)

🪛 eslint

[error] 375-375: 'response' is never reassigned. Use 'const' instead.

(prefer-const)


385-386: 🛠️ Refactor suggestion

Handle Exceptions Properly Instead of Using Empty Catch Blocks

The empty catch block suppresses any errors thrown within the try block, which can hinder debugging and obscure underlying issues. It's important to handle exceptions properly by logging the error or implementing fallback logic.

Apply this change:

   } catch (e) {
-    /* empty */
+    console.error('Error fetching user type:', e);
   }

574-575: ⚠️ Potential issue

Ensure $ Is Defined or Replace With AngularJS Equivalent

The use of $ at lines 574-575 may cause runtime errors if jQuery is not included or $ is not defined. In AngularJS applications, it's recommended to use angular.element instead of $. Please replace $ with angular.element, or ensure that jQuery is properly loaded in the application.

Apply this change:

     rolesFactory.whenReady().then(() => {
       $rootScope.requestConfig(loginData);
-      $('double-bounce-spinner').remove();
-      $('#wrapper').removeClass('fade');
+      angular.element('double-bounce-spinner').remove();
+      angular.element('#wrapper').removeClass('fade');
     });
🧰 Tools
🪛 eslint

[error] 574-574: '$' is not defined.

(no-undef)


[error] 575-575: '$' is not defined.

(no-undef)


578-578: 🛠️ Refactor suggestion

Avoid Variable Shadowing by Renaming Parameters

The parameter res in the inner .then(res => { at line 585 shadows the res variable from the outer scope at line 578. Variable shadowing can lead to confusion and potential bugs. Please rename the inner parameter to avoid this issue.

Apply this change:

-      preStart().then(res => {
+      preStart().then(preStartResult => {
           if (preStartResult === 'redirected') {
             return;
           }
           if (preStartResult === 'warn') {
             $rootScope.noHttps = true;
           }
-          getUserType(rolesFactory).then(res => {
+          getUserType(rolesFactory).then(userTypeResult => {
               if (userTypeResult === 'login') {

Also applies to: 585-585

🧰 Tools
🪛 eslint

[error] 578-578: Expected parentheses around arrow function argument.

(arrow-parens)


589-590: ⚠️ Potential issue

Ensure $ Is Defined or Replace With AngularJS Equivalent

The use of $ at lines 589-590 may cause runtime errors if jQuery is not included or $ is not defined. Consistent with AngularJS best practices, please replace $ with angular.element, or ensure that jQuery is properly integrated into the project.

Apply this change:

             $rootScope.showLoginModal = true;
             $rootScope.$apply();
-            $('double-bounce-spinner').remove();
-            $('#wrapper').removeClass('fade');
+            angular.element('double-bounce-spinner').remove();
+            angular.element('#wrapper').removeClass('fade');
           }
         });
🧰 Tools
🪛 eslint

[error] 589-589: '$' is not defined.

(no-undef)


[error] 590-590: '$' is not defined.

(no-undef)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a51203e and 7e8a0c0.

📒 Files selected for processing (2)
  • app/scripts/app.js (6 hunks)
  • app/scripts/react-directives/users/pageStore.js (1 hunks)
🧰 Additional context used
🪛 eslint
app/scripts/app.js

[error] 96-96: Cannot use import declarations in modules that export using CommonJS (module.exports = 'foo' or exports.bar = 'hi')

(import/no-import-module-exports)


[error] 97-97: Cannot use import declarations in modules that export using CommonJS (module.exports = 'foo' or exports.bar = 'hi')

(import/no-import-module-exports)


[error] 356-356: Identifier 'https_domain' is not in camel case.

(camelcase)


[error] 357-357: Identifier 'https_domain' is not in camel case.

(camelcase)


[error] 362-362: Identifier 'https_domain' is not in camel case.

(camelcase)


[error] 373-373: 'rolesFactory' is already declared in the upper scope on line 52 column 8.

(no-shadow)


[error] 375-375: 'response' is never reassigned. Use 'const' instead.

(prefer-const)


[error] 425-425: 'rolesFactory' is already declared in the upper scope on line 52 column 8.

(no-shadow)


[error] 574-574: '$' is not defined.

(no-undef)


[error] 575-575: '$' is not defined.

(no-undef)


[error] 578-578: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 583-583: Assignment to property of function parameter '$rootScope'.

(no-param-reassign)


[error] 585-585: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 585-585: 'res' is already declared in the upper scope on line 578 column 23.

(no-shadow)


[error] 587-587: Assignment to property of function parameter '$rootScope'.

(no-param-reassign)


[error] 589-589: '$' is not defined.

(no-undef)


[error] 590-590: '$' is not defined.

(no-undef)

app/scripts/react-directives/users/pageStore.js

[error] 1-238: File has too many classes (2). Maximum allowed is 1.

(max-classes-per-file)


[error] 95-95: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 96-96: Expected no linebreak before this expression.

(implicit-arrow-linebreak)


[error] 97-98: Missing trailing comma.

(comma-dangle)


[error] 98-99: Missing trailing comma.

(comma-dangle)


[error] 99-99: Unexpected newline before ')'.

(function-paren-newline)


[error] 104-108: Redundant use of await on a return value.

(no-return-await)


[error] 107-108: Missing trailing comma.

(comma-dangle)


[error] 161-161: Expected no linebreak before this expression.

(implicit-arrow-linebreak)


[error] 167-168: Missing trailing comma.

(comma-dangle)


[error] 168-168: Unexpected newline before ')'.

(function-paren-newline)


[error] 173-173: Unexpected use of 'location'.

(no-restricted-globals)


[error] 176-176: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 188-188: 'modifiedUser' is never reassigned. Use 'const' instead.

(prefer-const)


[error] 193-193: Expected no linebreak before this expression.

(implicit-arrow-linebreak)


[error] 199-200: Missing trailing comma.

(comma-dangle)


[error] 200-200: Unexpected newline before ')'.

(function-paren-newline)


[error] 204-204: Assignment to property of function parameter 'user'.

(no-param-reassign)


[error] 205-205: Assignment to property of function parameter 'user'.

(no-param-reassign)


[error] 217-218: Missing trailing comma.

(comma-dangle)


[error] 222-222: Expected '===' and instead saw '=='.

(eqeqeq)


[error] 224-224: Expected no linebreak before this expression.

(implicit-arrow-linebreak)


[error] 226-227: Missing trailing comma.

(comma-dangle)


[error] 227-227: Unexpected newline before ')'.

(function-paren-newline)


[error] 232-232: Expected parentheses around arrow function argument.

(arrow-parens)

🪛 Biome (1.9.4)
app/scripts/app.js

[error] 375-375: This let declares a variable that is only assigned once.

'response' is never reassigned.

Safe fix: Use const instead.

(lint/style/useConst)

app/scripts/react-directives/users/pageStore.js

[error] 1-1: Redundant use strict directive.

The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.

(lint/suspicious/noRedundantUseStrict)


[error] 188-188: This let declares a variable that is only assigned once.

'modifiedUser' is never reassigned.

Safe fix: Use const instead.

(lint/style/useConst)

🪛 GitHub Check: Codacy Static Code Analysis
app/scripts/app.js

[warning] 362-362: app/scripts/app.js#L362
Dangerous location.href assignment can lead to XSS. Please use escape(encodeURI(https://${https_domain})) as a wrapper for escaping

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Codacy Static Code Analysis

app/scripts/react-directives/users/pageStore.js Outdated Show resolved Hide resolved
app/scripts/react-directives/users/pageStore.js Outdated Show resolved Hide resolved
app/scripts/react-directives/users/pageStore.js Outdated Show resolved Hide resolved
app/scripts/app.js Outdated Show resolved Hide resolved
Copy link

@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: 4

📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7e8a0c0 and 1af0d64.

📒 Files selected for processing (3)
  • app/scripts/app.js (8 hunks)
  • app/scripts/react-directives/users/pageStore.js (1 hunks)
  • app/scripts/services/roles.factory.js (2 hunks)
🧰 Additional context used
🪛 eslint
app/scripts/app.js

[error] 52-52: Cannot use import declarations in modules that export using CommonJS (module.exports = 'foo' or exports.bar = 'hi')

(import/no-import-module-exports)


[error] 96-96: Cannot use import declarations in modules that export using CommonJS (module.exports = 'foo' or exports.bar = 'hi')

(import/no-import-module-exports)


[error] 97-97: Cannot use import declarations in modules that export using CommonJS (module.exports = 'foo' or exports.bar = 'hi')

(import/no-import-module-exports)


[error] 341-341: '&&' should be placed at the beginning of the line.

(operator-linebreak)


[error] 342-342: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 342-342: Missing radix parameter.

(radix)


[error] 342-342: Missing radix parameter.

(radix)


[error] 577-577: 'angular' is not defined.

(no-undef)


[error] 578-578: 'angular' is not defined.

(no-undef)


[error] 581-581: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 586-586: Assignment to property of function parameter '$rootScope'.

(no-param-reassign)


[error] 588-588: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 590-590: Assignment to property of function parameter '$rootScope'.

(no-param-reassign)


[error] 592-592: 'angular' is not defined.

(no-undef)


[error] 593-593: 'angular' is not defined.

(no-undef)

app/scripts/react-directives/users/pageStore.js

[error] 1-238: File has too many classes (2). Maximum allowed is 1.

(max-classes-per-file)


[error] 95-95: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 96-96: Expected no linebreak before this expression.

(implicit-arrow-linebreak)


[error] 97-98: Missing trailing comma.

(comma-dangle)


[error] 98-99: Missing trailing comma.

(comma-dangle)


[error] 99-99: Unexpected newline before ')'.

(function-paren-newline)


[error] 107-108: Missing trailing comma.

(comma-dangle)


[error] 161-161: Expected no linebreak before this expression.

(implicit-arrow-linebreak)


[error] 167-168: Missing trailing comma.

(comma-dangle)


[error] 168-168: Unexpected newline before ')'.

(function-paren-newline)


[error] 173-173: Unexpected use of 'location'.

(no-restricted-globals)


[error] 176-176: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 193-193: Expected no linebreak before this expression.

(implicit-arrow-linebreak)


[error] 199-200: Missing trailing comma.

(comma-dangle)


[error] 200-200: Unexpected newline before ')'.

(function-paren-newline)


[error] 204-204: Assignment to property of function parameter 'user'.

(no-param-reassign)


[error] 205-205: Assignment to property of function parameter 'user'.

(no-param-reassign)


[error] 217-218: Missing trailing comma.

(comma-dangle)


[error] 224-224: Expected no linebreak before this expression.

(implicit-arrow-linebreak)


[error] 226-227: Missing trailing comma.

(comma-dangle)


[error] 227-227: Unexpected newline before ')'.

(function-paren-newline)


[error] 232-232: Expected parentheses around arrow function argument.

(arrow-parens)

app/scripts/services/roles.factory.js

[error] 6-6: Expected newline after "ngInject" directive.

(lines-around-directive)


[error] 7-7: Unexpected var, use let or const instead.

(no-var)


[error] 11-11: Unexpected dangling '_' in '_ROLE_ONE'.

(no-underscore-dangle)


[error] 16-16: Unexpected dangling '_' in '_ROLE_TWO'.

(no-underscore-dangle)


[error] 21-21: Unexpected dangling '_' in '_ROLE_THREE'.

(no-underscore-dangle)


[error] 37-37: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 44-44: Expected parentheses around arrow function argument.

(arrow-parens)

🪛 Biome (1.9.4)
app/scripts/app.js

[error] 342-342: Use Number.parseInt instead of the equivalent global.

ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.

(lint/style/useNumberNamespace)


[error] 342-342: Use Number.parseInt instead of the equivalent global.

ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.

(lint/style/useNumberNamespace)

app/scripts/react-directives/users/pageStore.js

[error] 1-1: Redundant use strict directive.

The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.

(lint/suspicious/noRedundantUseStrict)

app/scripts/services/roles.factory.js

[error] 7-7: Use let or const instead of var.

A variable declared with var is accessible in the whole body of the function. Thus, the variable can be accessed before its initialization and outside the block where it is declared.
See MDN web docs for more details.
Unsafe fix: Use 'const' instead.

(lint/style/noVar)

🪛 GitHub Check: Codacy Static Code Analysis
app/scripts/app.js

[warning] 341-341: app/scripts/app.js#L341
Unsafe Regular Expression


[warning] 365-365: app/scripts/app.js#L365
Dangerous location.href assignment can lead to XSS. Please use escape(encodeURI(https://${httpsDomain})) as a wrapper for escaping


[failure] 577-577: app/scripts/app.js#L577
'angular' is not defined.


[failure] 578-578: app/scripts/app.js#L578
'angular' is not defined.

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (5)
app/scripts/react-directives/users/pageStore.js (3)

1-1: Remove redundant 'use strict' directive in ES6 module

In ES6 modules, strict mode is enabled by default. Therefore, the 'use strict' directive at the beginning of the file is unnecessary and can be safely removed.

Apply this diff:

-'use strict';
🧰 Tools
🪛 Biome (1.9.4)

[error] 1-1: Redundant use strict directive.

The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.

(lint/suspicious/noRedundantUseStrict)

🪛 eslint

[error] 1-238: File has too many classes (2). Maximum allowed is 1.

(max-classes-per-file)


204-205: Avoid mutating function parameters

Modifying properties of a function parameter (user) can lead to unintended side effects and make the code harder to maintain. It's better practice to create a new object or clone the parameter before making modifications.

Apply this diff to avoid mutating the user parameter:

        // Create a new user object with updated properties without mutating the original
+       const updatedUser = {
+         ...user,
+         name: modifiedUser.name,
+         type: modifiedUser.type,
+       };
+       // Update the user in the users array
+       const index = this.users.findIndex(u => u.id === user.id);
+       if (index >= 0) {
+         this.users[index] = updatedUser;
+         sortUsers(this.users);
+       }
-       user.name = modifiedUser.name;
-       user.type = modifiedUser.type;
-       sortUsers(this.users);
🧰 Tools
🪛 eslint

[error] 204-204: Assignment to property of function parameter 'user'.

(no-param-reassign)


[error] 205-205: Assignment to property of function parameter 'user'.

(no-param-reassign)


161-169: 🛠️ Refactor suggestion

Handle promise chain correctly by returning the fetch result

In the addUser method, the fetchWrapper function is used, but the promise returned by fetch is not awaited within fetchWrapper, potentially leading to unexpected behavior. Consider ensuring that the fetchWrapper function properly returns the result of the fetch.

Apply this diff to modify fetchWrapper:

      async fetchWrapper(fetchFn) {
        try {
          this.pageWrapperStore.clearError();
          this.pageWrapperStore.setLoading(true);
-         const res = await fetchFn();
+         const res = await fetchFn();
+         if (!res) {
+           throw new Error('No response from fetch');
+         }
          if (res.ok) {
            this.pageWrapperStore.setLoading(false);
            return res;
          }
          this.processFetchError(res);
        } catch (error) {
          this.pageWrapperStore.setError(error);
        }
        this.pageWrapperStore.setLoading(false);
        return null;
      }

Likely invalid or redundant comment.

🧰 Tools
🪛 eslint

[error] 161-161: Expected no linebreak before this expression.

(implicit-arrow-linebreak)


[error] 167-168: Missing trailing comma.

(comma-dangle)


[error] 168-168: Unexpected newline before ')'.

(function-paren-newline)

app/scripts/services/roles.factory.js (1)

44-44: Correct the parameter name to onlyRoleGreaterThanOrEqual

The parameter onlyRoleGreatThanOrEqual appears to have a typo. It should be onlyRoleGreaterThanOrEqual for clarity and consistency.

Apply this diff:

-  roles.checkRights = onlyRoleGreatThanOrEqual => roles.current.role >= onlyRoleGreatThanOrEqual;
+  roles.checkRights = onlyRoleGreaterThanOrEqual => roles.current.role >= onlyRoleGreaterThanOrEqual;
🧰 Tools
🪛 eslint

[error] 44-44: Expected parentheses around arrow function argument.

(arrow-parens)

app/scripts/app.js (1)

592-593: Ensure angular is defined or imported

The use of angular.element may cause a runtime error if angular is not defined in the scope. Ensure that angular is available globally or consider importing it if necessary.

Please confirm that angular is globally available in this context. If not, import it at the beginning of the file:

+import angular from 'angular';
🧰 Tools
🪛 eslint

[error] 592-592: 'angular' is not defined.

(no-undef)


[error] 593-593: 'angular' is not defined.

(no-undef)

app/scripts/react-directives/users/pageStore.js Outdated Show resolved Hide resolved
app/scripts/services/roles.factory.js Show resolved Hide resolved
app/scripts/app.js Outdated Show resolved Hide resolved
app/scripts/app.js Outdated Show resolved Hide resolved
Copy link

@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

📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1af0d64 and 236d89c.

📒 Files selected for processing (2)
  • app/scripts/app.js (8 hunks)
  • app/scripts/react-directives/users/pageStore.js (1 hunks)
🧰 Additional context used
🪛 eslint
app/scripts/app.js

[error] 52-52: Cannot use import declarations in modules that export using CommonJS (module.exports = 'foo' or exports.bar = 'hi')

(import/no-import-module-exports)


[error] 96-96: Cannot use import declarations in modules that export using CommonJS (module.exports = 'foo' or exports.bar = 'hi')

(import/no-import-module-exports)


[error] 97-97: Cannot use import declarations in modules that export using CommonJS (module.exports = 'foo' or exports.bar = 'hi')

(import/no-import-module-exports)


[error] 344-344: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 346-346: Unexpected use of 'isNaN'. Use Number.isNaN instead https://github.com/airbnb/javascript#standard-library--isnan

(no-restricted-globals)


[error] 581-581: 'angular' is not defined.

(no-undef)


[error] 582-582: 'angular' is not defined.

(no-undef)


[error] 585-585: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 590-590: Assignment to property of function parameter '$rootScope'.

(no-param-reassign)


[error] 592-592: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 594-594: Assignment to property of function parameter '$rootScope'.

(no-param-reassign)


[error] 596-596: 'angular' is not defined.

(no-undef)


[error] 597-597: 'angular' is not defined.

(no-undef)

app/scripts/react-directives/users/pageStore.js

[error] 1-235: File has too many classes (2). Maximum allowed is 1.

(max-classes-per-file)


[error] 93-93: Unexpected lexical declaration in case block.

(no-case-declarations)


[error] 95-96: Missing trailing comma.

(comma-dangle)


[error] 104-105: Missing trailing comma.

(comma-dangle)


[error] 158-158: Expected no linebreak before this expression.

(implicit-arrow-linebreak)


[error] 164-165: Missing trailing comma.

(comma-dangle)


[error] 165-165: Unexpected newline before ')'.

(function-paren-newline)


[error] 170-170: Unexpected use of 'location'.

(no-restricted-globals)


[error] 173-173: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 190-190: Expected no linebreak before this expression.

(implicit-arrow-linebreak)


[error] 196-197: Missing trailing comma.

(comma-dangle)


[error] 197-197: Unexpected newline before ')'.

(function-paren-newline)


[error] 201-201: Assignment to property of function parameter 'user'.

(no-param-reassign)


[error] 202-202: Assignment to property of function parameter 'user'.

(no-param-reassign)


[error] 214-215: Missing trailing comma.

(comma-dangle)


[error] 221-221: Expected no linebreak before this expression.

(implicit-arrow-linebreak)


[error] 223-224: Missing trailing comma.

(comma-dangle)


[error] 224-224: Unexpected newline before ')'.

(function-paren-newline)


[error] 229-229: Expected parentheses around arrow function argument.

(arrow-parens)

🪛 Biome (1.9.4)
app/scripts/app.js

[error] 345-345: Use Number.parseInt instead of the equivalent global.

ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.

(lint/style/useNumberNamespace)


[error] 346-346: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.

See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.

(lint/suspicious/noGlobalIsNan)

app/scripts/react-directives/users/pageStore.js

[error] 1-1: Redundant use strict directive.

The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.

(lint/suspicious/noRedundantUseStrict)


[error] 93-93: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.

The declaration is defined in this switch clause:

Unsafe fix: Wrap the declaration in a block.

(lint/correctness/noSwitchDeclarations)

🪛 GitHub Check: Codacy Static Code Analysis
app/scripts/app.js

[warning] 369-369: app/scripts/app.js#L369
Dangerous location.href assignment can lead to XSS. Please use escape(encodeURI(https://${httpsDomain})) as a wrapper for escaping


[failure] 581-581: app/scripts/app.js#L581
'angular' is not defined.


[failure] 582-582: app/scripts/app.js#L582
'angular' is not defined.

app/scripts/react-directives/users/pageStore.js

[warning] 93-93: app/scripts/react-directives/users/pageStore.js#L93
Unexpected lexical declaration in case block.

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (1)
app/scripts/app.js (1)

581-582: Ensure angular is defined before usage

The angular object is used but not explicitly imported or guaranteed to be available in this scope. This might lead to a ReferenceError at runtime if angular is undefined. Verify that angular is globally available or consider importing it.

🧰 Tools
🪛 eslint

[error] 581-581: 'angular' is not defined.

(no-undef)


[error] 582-582: 'angular' is not defined.

(no-undef)

🪛 GitHub Check: Codacy Static Code Analysis

[failure] 581-581: app/scripts/app.js#L581
'angular' is not defined.


[failure] 582-582: app/scripts/app.js#L582
'angular' is not defined.

app/scripts/react-directives/users/pageStore.js Outdated Show resolved Hide resolved
app/scripts/app.js Show resolved Hide resolved
app/scripts/app.js Show resolved Hide resolved
Copy link

@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 comments (3)
app/scripts/controllers/scriptsController.js (3)

Line range hint 13-26: Remove or replace console.log statement.

Debug logging should not be present in production code. Consider using a proper logging service or removing the statement.

    whenMqttReady()
      .then(() => EditorProxy.List())
      .then(scripts => {
-        console.log('scripts', scripts);
        // пишу каждому временное имя
        this.scripts = scripts.map(script => {

Line range hint 29-37: Convert API documentation to JSDoc format.

The commented API documentation should be properly formatted using JSDoc for better maintainability and IDE support.

-    /*
-     get list
-     /rpc/v1/wbrules/Editor/List/wb-mqtt-homeui-uLo93IW6a0 {"id":1,"params":{}}
-     get one
-     /rpc/v1/wbrules/Editor/Load/wb-mqtt-homeui-Usf4VuT4Ba {"id":2,"params":{"path":"rules.js"}}
-     save
-     /rpc/v1/wbrules/Editor/Save/wb-mqtt-homeui-Usf4VuT4Ba {"id":3,"params":{"path":"rules.js","content": ....
-     */
+    /**
+     * API Endpoints:
+     * 
+     * Get list:
+     * @example
+     * GET /rpc/v1/wbrules/Editor/List/wb-mqtt-homeui-uLo93IW6a0
+     * Request: {"id":1,"params":{}}
+     * 
+     * Get one:
+     * @example
+     * GET /rpc/v1/wbrules/Editor/Load/wb-mqtt-homeui-Usf4VuT4Ba
+     * Request: {"id":2,"params":{"path":"rules.js"}}
+     * 
+     * Save:
+     * @example
+     * POST /rpc/v1/wbrules/Editor/Save/wb-mqtt-homeui-Usf4VuT4Ba
+     * Request: {"id":3,"params":{"path":"rules.js","content":"..."}}
+     */

Line range hint 42-51: Replace alert() with proper error handling.

The method uses a basic alert() for error handling when the injected errors service should be used instead.

    this.EditorProxy.Remove({ path: path }).then(
      script => {
        this.scripts.splice(index, 1);
      },
-     err => alert(err)
+     err => this.errors.showError('scripts.errors.delete', err)
    );
🧰 Tools
🪛 eslint

[error] 35-35: This line has a length of 110. Maximum allowed is 100.

(max-len)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 236d89c and a0a2288.

📒 Files selected for processing (3)
  • app/scripts/app.js (9 hunks)
  • app/scripts/controllers/scriptsController.js (4 hunks)
  • app/scripts/react-directives/users/pageStore.js (1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/scripts/controllers/scriptsController.js

[error] 2-3: Redundant use strict directive.

The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.

(lint/suspicious/noRedundantUseStrict)

app/scripts/react-directives/users/pageStore.js

[error] 1-1: Redundant use strict directive.

The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.

(lint/suspicious/noRedundantUseStrict)

app/scripts/app.js

[error] 347-347: Use Number.parseInt instead of the equivalent global.

ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.

(lint/style/useNumberNamespace)


[error] 348-348: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.

See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.

(lint/suspicious/noGlobalIsNan)

🪛 eslint
app/scripts/react-directives/users/pageStore.js

[error] 1-238: File has too many classes (2). Maximum allowed is 1.

(max-classes-per-file)


[error] 97-98: Missing trailing comma.

(comma-dangle)


[error] 107-108: Missing trailing comma.

(comma-dangle)


[error] 161-161: Expected no linebreak before this expression.

(implicit-arrow-linebreak)


[error] 167-168: Missing trailing comma.

(comma-dangle)


[error] 168-168: Unexpected newline before ')'.

(function-paren-newline)


[error] 173-173: Unexpected use of 'location'.

(no-restricted-globals)


[error] 176-176: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 193-193: Expected no linebreak before this expression.

(implicit-arrow-linebreak)


[error] 199-200: Missing trailing comma.

(comma-dangle)


[error] 200-200: Unexpected newline before ')'.

(function-paren-newline)


[error] 204-204: Assignment to property of function parameter 'user'.

(no-param-reassign)


[error] 205-205: Assignment to property of function parameter 'user'.

(no-param-reassign)


[error] 217-218: Missing trailing comma.

(comma-dangle)


[error] 224-224: Expected no linebreak before this expression.

(implicit-arrow-linebreak)


[error] 226-227: Missing trailing comma.

(comma-dangle)


[error] 227-227: Unexpected newline before ')'.

(function-paren-newline)


[error] 232-232: Expected parentheses around arrow function argument.

(arrow-parens)

app/scripts/app.js

[error] 54-54: Cannot use import declarations in modules that export using CommonJS (module.exports = 'foo' or exports.bar = 'hi')

(import/no-import-module-exports)


[error] 98-98: Cannot use import declarations in modules that export using CommonJS (module.exports = 'foo' or exports.bar = 'hi')

(import/no-import-module-exports)


[error] 99-99: Cannot use import declarations in modules that export using CommonJS (module.exports = 'foo' or exports.bar = 'hi')

(import/no-import-module-exports)


[error] 346-346: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 348-348: Unexpected use of 'isNaN'. Use Number.isNaN instead https://github.com/airbnb/javascript#standard-library--isnan

(no-restricted-globals)


[error] 589-589: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 594-594: Assignment to property of function parameter '$rootScope'.

(no-param-reassign)


[error] 596-596: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 598-598: Assignment to property of function parameter '$rootScope'.

(no-param-reassign)

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (4)
app/scripts/app.js (1)

348-348: 🛠️ Refactor suggestion

Use Number.isNaN Instead of Global isNaN

The global isNaN function can lead to unexpected results due to type coercion. Using Number.isNaN provides a more reliable check.

Apply this diff:

-    return !isNaN(parsed) && parsed >= 0 && parsed <= 255;
+    return !Number.isNaN(parsed) && parsed >= 0 && parsed <= 255;

Likely invalid or redundant comment.

🧰 Tools
🪛 Biome (1.9.4)

[error] 348-348: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.

See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.

(lint/suspicious/noGlobalIsNan)

🪛 eslint

[error] 348-348: Unexpected use of 'isNaN'. Use Number.isNaN instead https://github.com/airbnb/javascript#standard-library--isnan

(no-restricted-globals)

app/scripts/react-directives/users/pageStore.js (1)

1-238: Well-structured implementation of user management functionality

The code provides comprehensive user management features with proper state handling using MobX. The classes and methods are well-defined, and the code is clean and readable.

🧰 Tools
🪛 Biome (1.9.4)

[error] 1-1: Redundant use strict directive.

The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.

(lint/suspicious/noRedundantUseStrict)

🪛 eslint

[error] 1-238: File has too many classes (2). Maximum allowed is 1.

(max-classes-per-file)


[error] 97-98: Missing trailing comma.

(comma-dangle)


[error] 107-108: Missing trailing comma.

(comma-dangle)


[error] 161-161: Expected no linebreak before this expression.

(implicit-arrow-linebreak)


[error] 167-168: Missing trailing comma.

(comma-dangle)


[error] 168-168: Unexpected newline before ')'.

(function-paren-newline)


[error] 173-173: Unexpected use of 'location'.

(no-restricted-globals)


[error] 176-176: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 193-193: Expected no linebreak before this expression.

(implicit-arrow-linebreak)


[error] 199-200: Missing trailing comma.

(comma-dangle)


[error] 200-200: Unexpected newline before ')'.

(function-paren-newline)


[error] 204-204: Assignment to property of function parameter 'user'.

(no-param-reassign)


[error] 205-205: Assignment to property of function parameter 'user'.

(no-param-reassign)


[error] 217-218: Missing trailing comma.

(comma-dangle)


[error] 224-224: Expected no linebreak before this expression.

(implicit-arrow-linebreak)


[error] 226-227: Missing trailing comma.

(comma-dangle)


[error] 227-227: Unexpected newline before ')'.

(function-paren-newline)


[error] 232-232: Expected parentheses around arrow function argument.

(arrow-parens)

app/scripts/controllers/scriptsController.js (2)

27-29: LGTM! Good transition to async rights verification.

The switch from synchronous to asynchronous rights verification improves the application's responsiveness.


54-54: LGTM! Clean module export.

The module definition and controller registration follow AngularJS best practices.

app/scripts/app.js Show resolved Hide resolved
app/scripts/app.js Show resolved Hide resolved
app/scripts/controllers/scriptsController.js Show resolved Hide resolved
Copy link

@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: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 645bed2 and 4dac73b.

📒 Files selected for processing (1)
  • app/scripts/app.js (9 hunks)
🧰 Additional context used
🪛 eslint
app/scripts/app.js

[error] 54-54: Cannot use import declarations in modules that export using CommonJS (module.exports = 'foo' or exports.bar = 'hi')

(import/no-import-module-exports)


[error] 98-98: Cannot use import declarations in modules that export using CommonJS (module.exports = 'foo' or exports.bar = 'hi')

(import/no-import-module-exports)


[error] 99-99: Cannot use import declarations in modules that export using CommonJS (module.exports = 'foo' or exports.bar = 'hi')

(import/no-import-module-exports)


[error] 346-346: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 348-348: Unexpected use of 'isNaN'. Use Number.isNaN instead https://github.com/airbnb/javascript#standard-library--isnan

(no-restricted-globals)


[error] 594-594: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 599-599: Assignment to property of function parameter '$rootScope'.

(no-param-reassign)


[error] 601-601: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 603-603: Assignment to property of function parameter '$rootScope'.

(no-param-reassign)

🪛 Biome (1.9.4)
app/scripts/app.js

[error] 347-347: Use Number.parseInt instead of the equivalent global.

ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.

(lint/style/useNumberNamespace)


[error] 348-348: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.

See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.

(lint/suspicious/noGlobalIsNan)

🪛 GitHub Check: Codacy Static Code Analysis
app/scripts/app.js

[warning] 377-377: app/scripts/app.js#L377
Dangerous location.href assignment can lead to XSS. Please use escape(https://${httpsDomain}) as a wrapper for escaping

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (6)
app/scripts/app.js (6)

166-166: LGTM: Service registration follows Angular patterns.

The rolesFactory service is correctly registered using Angular's dependency injection system.


269-271: LGTM: Directive registrations are properly structured.

The new directives are correctly registered using Angular's directive system.


341-350: LGTM: IP validation logic is correct.

The IP validation function properly checks for valid IPv4 addresses by validating each octet.

🧰 Tools
🪛 Biome (1.9.4)

[error] 347-347: Use Number.parseInt instead of the equivalent global.

ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.

(lint/style/useNumberNamespace)


[error] 348-348: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.

See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.

(lint/suspicious/noGlobalIsNan)

🪛 eslint

[error] 346-346: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 348-348: Unexpected use of 'isNaN'. Use Number.isNaN instead https://github.com/airbnb/javascript#standard-library--isnan

(no-restricted-globals)


389-405: LGTM: User type handling with proper fallback.

The function properly handles user type assignment with appropriate error handling and fallback to 'user' role.


588-609: LGTM: Proper initialization sequence.

The initialization sequence correctly:

  1. Waits for roles to be ready
  2. Handles HTTPS redirection
  3. Manages user type initialization
  4. Shows login modal when needed
🧰 Tools
🪛 eslint

[error] 594-594: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 599-599: Assignment to property of function parameter '$rootScope'.

(no-param-reassign)


[error] 601-601: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 603-603: Assignment to property of function parameter '$rootScope'.

(no-param-reassign)


522-529: LGTM: Robust login URL handling.

The login URL construction properly:

  • Uses the URL API for safe URL manipulation
  • Handles development environment appropriately
  • Correctly switches protocols for WebSocket connections

app/scripts/app.js Outdated Show resolved Hide resolved
Copy link

@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: ASSERTIVE
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4dac73b and d29caa0.

📒 Files selected for processing (1)
  • app/scripts/app.js (9 hunks)
🧰 Additional context used
🪛 eslint
app/scripts/app.js

[error] 54-54: Cannot use import declarations in modules that export using CommonJS (module.exports = 'foo' or exports.bar = 'hi')

(import/no-import-module-exports)


[error] 98-98: Cannot use import declarations in modules that export using CommonJS (module.exports = 'foo' or exports.bar = 'hi')

(import/no-import-module-exports)


[error] 99-99: Cannot use import declarations in modules that export using CommonJS (module.exports = 'foo' or exports.bar = 'hi')

(import/no-import-module-exports)


[error] 102-102: Cannot use import declarations in modules that export using CommonJS (module.exports = 'foo' or exports.bar = 'hi')

(import/no-import-module-exports)


[error] 105-105: Cannot use import declarations in modules that export using CommonJS (module.exports = 'foo' or exports.bar = 'hi')

(import/no-import-module-exports)


[error] 107-107: Cannot use import declarations in modules that export using CommonJS (module.exports = 'foo' or exports.bar = 'hi')

(import/no-import-module-exports)


[error] 107-107: lodash/escape import should occur before import of ./services/errors

(import/order)


[error] 348-348: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 350-350: Unexpected use of 'isNaN'. Use Number.isNaN instead https://github.com/airbnb/javascript#standard-library--isnan

(no-restricted-globals)


[error] 589-589: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 594-594: Assignment to property of function parameter '$rootScope'.

(no-param-reassign)


[error] 596-596: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 598-598: Assignment to property of function parameter '$rootScope'.

(no-param-reassign)

🪛 Biome (1.9.4)
app/scripts/app.js

[error] 107-107: Do not shadow the global "escape" property.

Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.

(lint/suspicious/noShadowRestrictedNames)


[error] 349-349: Use Number.parseInt instead of the equivalent global.

ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.

(lint/style/useNumberNamespace)


[error] 350-350: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.

See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.

(lint/suspicious/noGlobalIsNan)

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Codacy Static Code Analysis

app/scripts/app.js Outdated Show resolved Hide resolved
app/scripts/app.js Outdated Show resolved Hide resolved
@KraPete KraPete mentioned this pull request Jan 23, 2025
Copy link

@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: 22

📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d29caa0 and f30dc8b.

📒 Files selected for processing (11)
  • app/scripts/app.js (9 hunks)
  • app/scripts/i18n/app/en.json (2 hunks)
  • app/scripts/i18n/app/ru.json (2 hunks)
  • app/scripts/react-directives/common.jsx (1 hunks)
  • app/scripts/react-directives/components/modals/formModal.jsx (1 hunks)
  • app/scripts/react-directives/forms/forms.jsx (5 hunks)
  • app/scripts/react-directives/forms/stringStore.js (2 hunks)
  • app/scripts/react-directives/login/loginModal.jsx (1 hunks)
  • app/scripts/react-directives/login/store.js (1 hunks)
  • app/styles/main.css (2 hunks)
  • webpack.config.js (5 hunks)
🧰 Additional context used
📓 Learnings (1)
app/scripts/app.js (2)
Learnt from: KraPete
PR: wirenboard/homeui#684
File: app/scripts/app.js:107-107
Timestamp: 2025-01-23T04:06:55.712Z
Learning: In the homeui codebase, shadowing deprecated global functions (like `escape`) is acceptable when the usage is clear and limited in scope, as demonstrated by the lodash escape import in app.js.
Learnt from: KraPete
PR: wirenboard/homeui#684
File: app/scripts/app.js:0-0
Timestamp: 2025-01-22T14:26:54.251Z
Learning: For the homeui application, strict domain validation for HTTPS redirects is not required as it operates in a trusted local network environment.
🪛 Biome (1.9.4)
app/scripts/react-directives/common.jsx

[error] 108-108: Template literals are preferred over string concatenation.

Unsafe fix: Use a template literal.

(lint/style/useTemplate)


[error] 108-108: Template literals are preferred over string concatenation.

Unsafe fix: Use a template literal.

(lint/style/useTemplate)

app/scripts/react-directives/components/modals/formModal.jsx

[error] 15-15: Template literals are preferred over string concatenation.

Unsafe fix: Use a template literal.

(lint/style/useTemplate)

app/scripts/app.js

[error] 107-107: Do not shadow the global "escape" property.

Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.

(lint/suspicious/noShadowRestrictedNames)


[error] 349-349: Use Number.parseInt instead of the equivalent global.

ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.

(lint/style/useNumberNamespace)


[error] 350-350: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.

See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.

(lint/suspicious/noGlobalIsNan)

🪛 eslint
app/scripts/react-directives/common.jsx

[error] 106-126: Function component is not a function declaration

(react/function-component-definition)


[error] 106-106: Expected a line break after this opening brace.

(object-curly-newline)


[error] 106-106: 'label' is missing in props validation

(react/prop-types)


[error] 106-106: 'type' is missing in props validation

(react/prop-types)


[error] 106-106: 'onClick' is missing in props validation

(react/prop-types)


[error] 106-106: 'disabled' is missing in props validation

(react/prop-types)


[error] 106-106: 'additionalStyles' is missing in props validation

(react/prop-types)


[error] 106-106: 'icon' is missing in props validation

(react/prop-types)


[error] 106-106: 'title' is missing in props validation

(react/prop-types)


[error] 106-106: 'form' is missing in props validation

(react/prop-types)


[error] 106-106: Expected a line break before this closing brace.

(object-curly-newline)


[error] 107-107: There should be no line break before or after '='.

(operator-linebreak)


[error] 108-108: Unexpected string concatenation.

(prefer-template)


[error] 108-108: Unnecessary use of conditional expression for default assignment.

(no-unneeded-ternary)


[error] 108-108: Unexpected string concatenation.

(prefer-template)

app/scripts/react-directives/login/loginModal.jsx

[error] 1-1: '/home/jailuser/git/node_modules/react/index.js' imported multiple times.

(import/no-duplicates)


[error] 13-13: react import should occur before import of ../components/modals/modals

(import/order)


[error] 13-13: '/home/jailuser/git/node_modules/react/index.js' imported multiple times.

(import/no-duplicates)


[error] 24-24: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 30-30: Curly braces are unnecessary here.

(react/jsx-curly-brace-presence)


[error] 32-32: Curly braces are unnecessary here.

(react/jsx-curly-brace-presence)


[error] 44-44: Curly braces are unnecessary here.

(react/jsx-curly-brace-presence)


[error] 45-45: Curly braces are unnecessary here.

(react/jsx-curly-brace-presence)


[error] 53-53: 'store' is missing in props validation

(react/prop-types)

webpack.config.js

[error] 1-1: Definition for rule '@typescript-eslint/no-var-requires' was not found.

(@typescript-eslint/no-var-requires)

app/scripts/react-directives/components/modals/formModal.jsx

[error] 15-15: Unexpected string concatenation.

(prefer-template)


[error] 16-16: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 33-33: Curly braces are unnecessary here.

(react/jsx-curly-brace-presence)

app/scripts/app.js

[error] 54-54: Cannot use import declarations in modules that export using CommonJS (module.exports = 'foo' or exports.bar = 'hi')

(import/no-import-module-exports)


[error] 98-98: Cannot use import declarations in modules that export using CommonJS (module.exports = 'foo' or exports.bar = 'hi')

(import/no-import-module-exports)


[error] 99-99: Cannot use import declarations in modules that export using CommonJS (module.exports = 'foo' or exports.bar = 'hi')

(import/no-import-module-exports)


[error] 102-102: Cannot use import declarations in modules that export using CommonJS (module.exports = 'foo' or exports.bar = 'hi')

(import/no-import-module-exports)


[error] 105-105: Cannot use import declarations in modules that export using CommonJS (module.exports = 'foo' or exports.bar = 'hi')

(import/no-import-module-exports)


[error] 107-107: Cannot use import declarations in modules that export using CommonJS (module.exports = 'foo' or exports.bar = 'hi')

(import/no-import-module-exports)


[error] 107-107: lodash/escape import should occur before import of ./services/errors

(import/order)


[error] 107-107: 'escape' is defined but never used.

(no-unused-vars)


[error] 348-348: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 350-350: Unexpected use of 'isNaN'. Use Number.isNaN instead https://github.com/airbnb/javascript#standard-library--isnan

(no-restricted-globals)


[error] 618-618: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 623-623: Assignment to property of function parameter '$rootScope'.

(no-param-reassign)


[error] 625-625: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 627-627: Assignment to property of function parameter '$rootScope'.

(no-param-reassign)

app/scripts/react-directives/forms/forms.jsx

[error] 53-53: 'store' is missing in props validation

(react/prop-types)


[error] 53-65: Unexpected block statement surrounding arrow body; move the returned value immediately after the =>.

(arrow-body-style)


[error] 57-57: 'store.value' is missing in props validation

(react/prop-types)


[error] 57-57: 'store.value' is missing in props validation

(react/prop-types)


[error] 58-58: 'store.placeholder' is missing in props validation

(react/prop-types)


[error] 59-59: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 59-59: 'store.setValue' is missing in props validation

(react/prop-types)


[error] 60-60: 'store.readOnly' is missing in props validation

(react/prop-types)


[error] 65-66: Missing trailing comma.

(comma-dangle)


[error] 159-159: 'param' is missing in props validation

(react/prop-types)


[error] 159-159: 'paramName' is missing in props validation

(react/prop-types)


[error] 170-170: 'param.type' is missing in props validation

(react/prop-types)


[error] 173-173: 'param.type' is missing in props validation

(react/prop-types)

app/scripts/react-directives/login/store.js

[error] 78-78: Expected parentheses around arrow function argument.

(arrow-parens)

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (24)
app/styles/main.css (1)

1253-1256: LGTM!

The implementation of right-aligned dropdown menus follows Bootstrap's standard approach.

webpack.config.js (1)

102-108: Nice job separating font assets.

This distinctive rule for fonts keeps the build organized. As above, connecting a hash to the filename can further aid cache management.

app/scripts/react-directives/forms/stringStore.js (3)

22-22: LGTM! Proper MobX observable declaration.

The readOnly property is correctly declared as an observable, following MobX patterns and best practices.


30-30: LGTM! Proper MobX action registration.

The setReadOnly action is correctly registered in makeObservable, maintaining MobX state management patterns.


Line range hint 1-76: Consider synchronization with authentication state.

Since this change is part of adding authentication, consider whether the readOnly state should be automatically synchronized with the user's authentication status or permissions. This might prevent potential state inconsistencies.

app/scripts/react-directives/login/store.js (3)

1-5: All good with initial imports.
No issues spotted in these lines.


6-42: Constructor & state initialization appear sound.
The usage of MobX’s makeObservable with action/observable properties is well-structured. The FormStore integration looks correct and supports easily extensible fields.


44-63: Consistency check on setter methods.
All setter methods (setLogin, setPassword, setError, setHttpWarning, clearError) straightforwardly modify observable properties. This is maintainable and matches the reactive architecture.

app/scripts/react-directives/login/loginModal.jsx (2)

15-23: Store usage is well-designed.
MobX’s observer HOC effectively wraps the component to react to changes in the store.


57-57: Export approach is fine.
No concerns here. Inline function with default export is feasible for this context.

app/scripts/i18n/app/en.json (3)

7-9: New error messages look appropriate.
These lines add clarifying prompts for administrative setup and HTTPS usage, aligning well with the PR’s objectives of enhancing security.


14-17: Logout button labeling.
The added logout label is succinct and consistent.


18-21: Role definitions aligned with new authentication logic.
Properly localized role labels help maintain clarity for administrative tasks.

app/scripts/i18n/app/ru.json (3)

7-9: Well-structured error messages with clear user guidance!

The new error messages effectively communicate security-related issues to users in Russian, with clear instructions for resolution.


14-17: Clear and concise button label!

The logout button label is appropriately translated and well-organized within the buttons section.


18-21: Well-structured role hierarchy labels!

The role labels follow a clear hierarchical structure and use standard terminology in Russian.

app/scripts/app.js (4)

612-616: LGTM! Proper initialization sequence.

The code correctly waits for roles to be ready before proceeding with configuration and UI updates.


618-633: LGTM! Well-structured initialization flow.

The code properly handles HTTPS redirection and login states, with appropriate UI feedback.

🧰 Tools
🪛 eslint

[error] 618-618: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 623-623: Assignment to property of function parameter '$rootScope'.

(no-param-reassign)


[error] 625-625: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 627-627: Assignment to property of function parameter '$rootScope'.

(no-param-reassign)


343-352: 🛠️ Refactor suggestion

Strengthen IP address validation

The current IP validation could be improved to handle edge cases:

  1. Validate octet format (no leading zeros)
  2. Use Number.parseInt and Number.isNaN for more precise validation

Apply this diff to improve validation:

 function isIp(host) {
   const ipComponents = host.split('.');
   if (ipComponents.length !== 4) {
     return false;
   }
   return ipComponents.every(num => {
-    const parsed = parseInt(num, 10);
-    return !isNaN(parsed) && parsed >= 0 && parsed <= 255;
+    if (!/^(0|[1-9]\d*)$/.test(num)) return false;
+    const parsed = Number.parseInt(num, 10);
+    return !Number.isNaN(parsed) && parsed >= 0 && parsed <= 255;
   });
 }

Likely invalid or redundant comment.

🧰 Tools
🪛 Biome (1.9.4)

[error] 349-349: Use Number.parseInt instead of the equivalent global.

ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.

(lint/style/useNumberNamespace)


[error] 350-350: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.

See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.

(lint/suspicious/noGlobalIsNan)

🪛 eslint

[error] 348-348: Expected parentheses around arrow function argument.

(arrow-parens)


[error] 350-350: Unexpected use of 'isNaN'. Use Number.isNaN instead https://github.com/airbnb/javascript#standard-library--isnan

(no-restricted-globals)


354-356: 🧹 Nitpick (assertive)

Enhance local domain validation

The current implementation could be strengthened to ensure valid domain names.

Apply this diff to improve validation:

 function isLocalDomain(host) {
-  return host.endsWith('.local');
+  const validDomain = /^[a-zA-Z0-9]([a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(\.[a-zA-Z0-9]([a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)*\.local$/;
+  return validDomain.test(host);
 }

Likely invalid or redundant comment.

app/scripts/react-directives/components/modals/formModal.jsx (1)

25-29: Form-based approach looks great.

Wrapping MakeFormFields in a <form> with an onSubmit handler properly leverages HTML form submission and can simplify validation flows.

app/scripts/react-directives/forms/forms.jsx (3)

91-91: Enabling read-only mode is well-implemented.

disabled={store.readOnly} ensures correct handling of the read-only state for FormSelect.


Line range hint 159-192: Forward ref usage is appropriate here.

By forwarding refs in FormEditor, you enable advanced focus management and programmatic control of child inputs. This is a sound architectural decision.

🧰 Tools
🪛 eslint

[error] 167-167: 'param.type' is missing in props validation

(react/prop-types)


[error] 170-170: 'param.type' is missing in props validation

(react/prop-types)


[error] 173-173: 'param.type' is missing in props validation

(react/prop-types)


[error] 176-176: 'param.type' is missing in props validation

(react/prop-types)


194-200: Well-structured dynamic form field creation.

Conditionally applying the ref to the first field is a neat way to direct focus or capture the initial user input. This design is flexible and maintainable.

🧰 Tools
🪛 eslint

[error] 194-201: Unexpected block statement surrounding arrow body; move the returned value immediately after the =>.

(arrow-body-style)

app/styles/main.css Show resolved Hide resolved
webpack.config.js Show resolved Hide resolved
webpack.config.js Show resolved Hide resolved
webpack.config.js Show resolved Hide resolved
webpack.config.js Show resolved Hide resolved
app/scripts/react-directives/common.jsx Show resolved Hide resolved
app/scripts/react-directives/common.jsx Show resolved Hide resolved
app/scripts/react-directives/forms/forms.jsx Show resolved Hide resolved
app/scripts/react-directives/forms/forms.jsx Show resolved Hide resolved
KraPete and others added 4 commits January 24, 2025 11:47
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
@evgeny-boger
Copy link
Member

image
тут бы ссылку сразу на настройки

@evgeny-boger
Copy link
Member

image
это тоже можно ссылкой

@evgeny-boger
Copy link
Member

image
чот тут ещё так коряво у полей прописали, что браузер не предлагает запоминать пароль. Это прям критично

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.

3 participants