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

[BYOM] Support for Local, Private Endpoints #26475

Merged
merged 21 commits into from
Nov 21, 2024

Conversation

jonathansampson
Copy link
Contributor

@jonathansampson jonathansampson commented Nov 11, 2024

Resolves brave/brave-browser#39627

Included Scenarios

Derived from July 8th and July 18th comments.

  • Warn when a private IP/endpoint is used and the feature flag is disabled.

To satisfy this requirement, a modal will be displayed upon attempting to save a model configuration with a private endpoint, while the feature is disabled. The modal will give a brief explanation of the issue, and provide guidance on how to enable the feature.

image

  • Warn when a private IP/endpoint is used and the feature flag is enabled (with additional context about risks).

This requirement is addressed with a 🔓 icon (and label) displayed prominently beneath the server endpoint input component. This warning element is only displayed when the server endpoint is valid only due to the optional feature having been enabled.

image

  • Warn the user when they attempt to use a model that has an invalid endpoint. Do not permit messages to be sent under this scenario.

This pull request introduces a new error message for the chat context. If the model's server endpoint is invalid, the user will be informed. Furthermore, a "Configure" button is displayed to assist the user in reaching the model configuration UI.

image

Submitter Checklist:

  • I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Feature Disabled Scenario

  • Verify the feature is NOT enabled via brave://flags/#brave-ai-chat-allow-private-ips
  • Add a local network endpoint (e.g., http://10.198.1.12:11434/v1/chat/completions) in a new or existing model configuration.
    • You should see a warning about the endpoint being invalid (i.e., "Non-local endpoints…")
    • You should see a modal dialog (i.e., "Private IPs Not Allowed") upon clicking "Save model"

Feature Enabled Scenario

  • Verify the feature IS enabled
  • Add a local network endpoint in a new or existing model configuration
    • You should see a warning beneath the endpoint (i.e., "🔓 This endpoint is…")

Feature Disabled After Having Been Enabled

  • Verify that there is a properly configured model with a private endpoint
  • Verify that the feature has afterwards been disabled
  • Attempt to use the model in a Leo chat instance
    • You should see a warning in the Leo chat regarding the model's endpoint (i.e., "This model has an invalid endpoint…")

@github-actions github-actions bot added the CI/run-upstream-tests Run upstream unit and browser tests on Linux and Windows (otherwise only on Linux) label Nov 11, 2024
@jonathansampson jonathansampson changed the title Sampson issue 39627 private ip endpoint [AI Chat] Support for Local, Private Endpoints Nov 11, 2024
@jonathansampson jonathansampson marked this pull request as draft November 11, 2024 15:16
@jonathansampson jonathansampson changed the title [AI Chat] Support for Local, Private Endpoints [BYOM] Support for Local, Private Endpoints Nov 11, 2024
@jonathansampson jonathansampson force-pushed the sampson-issue-39627-private-ip-endpoint branch 2 times, most recently from cd57346 to 3738d8b Compare November 14, 2024 19:07
@github-actions github-actions bot added the CI/storybook-url Deploy storybook and provide a unique URL for each build label Nov 14, 2024
@jonathansampson jonathansampson force-pushed the sampson-issue-39627-private-ip-endpoint branch from b52b60a to 3eeef3b Compare November 18, 2024 14:46
@jonathansampson jonathansampson marked this pull request as ready for review November 18, 2024 15:09
@jonathansampson jonathansampson requested a review from a team as a code owner November 18, 2024 15:09
@jonathansampson jonathansampson added enhancement feature/settings feature/privacy Privacy-related feature that doesn't fall under other labels such as feature/shields javascript Pull requests that update Javascript code needs-security-review feature/leo Leo-related issues and features labels Nov 18, 2024
@diracdeltas diracdeltas requested review from thypon and removed request for diracdeltas November 18, 2024 17:06
@thypon
Copy link
Member

thypon commented Nov 18, 2024

Total change C4 scheme

C4Context
    title Private IP Addresses for Custom Model Endpoints

    Person(user, "User", "Brave browser user configuring custom AI models")
    
    System_Boundary(brave, "Brave Browser") {
        Container(ui, "Settings UI", "WebUI", "Handles model configuration and displays warnings")
        Container(validator, "Model Validator", "C++", "Validates model endpoints and configurations")
        Container(chat, "AI Chat Service", "C++", "Manages AI chat interactions and models")
        Container(flags, "Feature Flags", "C++", "Controls private IP feature availability")
    }
    
    System_Ext(private_endpoint, "Private Network Endpoint", "Custom AI model endpoint on local network")
    System_Ext(public_endpoint, "Public Endpoint", "Custom AI model endpoint on public internet")

    Rel(user, ui, "Configures custom model", "HTTPS")
    Rel(ui, validator, "Validates endpoint", "IPC")
    Rel(validator, flags, "Checks if private IPs allowed")
    Rel(chat, validator, "Validates model configuration")
    Rel(chat, private_endpoint, "Connects when allowed", "HTTP/HTTPS")
    Rel(chat, public_endpoint, "Connects", "HTTPS")
Loading

Model Endpoint Validation Flow

sequenceDiagram
    participant UI as Model Config UI
    participant Handler as Settings Handler
    participant Validator as Model Validator
    participant Features as Feature Flags

    UI->>Handler: validateModelEndpoint(url)
    Handler->>Validator: IsValidEndpoint(url)
    Validator->>Features: IsAllowPrivateIPsEnabled()
    Features-->>Validator: feature_enabled
    
    alt Is HTTPS or localhost
        Validator-->>Handler: true
    else Is private IP/hostname and feature enabled
        Validator->>Validator: IsValidPrivateHost() or IsValidPrivateIPAddress()
        Validator-->>Handler: true
    else
        Validator-->>Handler: false
    end
    
    Handler-->>UI: {isValid, isValidAsPrivateEndpoint, isValidDueToPrivateIPsFeature}
    
    alt isValid
        UI->>UI: Clear error state
    else isValidAsPrivateEndpoint
        UI->>UI: Show private IP warning modal
    else
        UI->>UI: Show invalid URL error
    end
Loading

Model Activation Flow

sequenceDiagram
    participant UI as Chat UI
    participant Handler as Conversation Handler
    participant Validator as Model Validator
    participant Features as Feature Flags

    UI->>Handler: ChangeModel(model_key)
    Handler->>Handler: GetModel(model_key)
    
    alt Is Custom Model
        Handler->>Validator: IsValidEndpoint(endpoint)
        Validator->>Features: IsAllowPrivateIPsEnabled()
        Features-->>Validator: feature_enabled
        
        alt Endpoint Valid
            Validator-->>Handler: true
            Handler->>Handler: SetAPIError(None)
            Handler->>Handler: InitEngine()
        else Endpoint Invalid
            Validator-->>Handler: false
            Handler->>Handler: SetAPIError(InvalidEndpointURL)
        end
    else
        Handler->>Handler: SetAPIError(None)
        Handler->>Handler: InitEngine()
    end
    
    Handler-->>UI: Update UI state
Loading

Copy link
Collaborator

@mkarolin mkarolin left a comment

Choose a reason for hiding this comment

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

strings++

Copy link
Member

@thypon thypon left a comment

Choose a reason for hiding this comment

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

We should probably just remove the .local check for the moment, or improve by resolving on the UI side and only storing the final IP

components/ai_chat/core/browser/model_validator.cc Outdated Show resolved Hide resolved
Moved some of the earlier endpoint tests into the newer param-based suite for better organization.
Current approach is to check the validity of the endpoint when the first human interaction takes place. If the model endpoint is invalid, an error is shown to the user. The displayed error invites the user to check their model's configuration.
Rather than waiting for the user to interact with the model, this change proactively notifies the user of an invalid model endpoint when the model is selected for use.
With the gap, there is an extra bit of padding above each model listing after the first. This gives the impression that the first model in the list is shorter (i.e., its bounding box size) than all that follow. Further, the gap causes asymmetry between the top and bottom padding on every model listing after the first.
We aim to provide an instructive error message to the user when they have provided an endpoint value that would only be valid with the enabling of optional private IPs.
This change introduces an alternative approach to endpoint validation. Some endpoints are only valid with the enabling of the optional brave-ai-chat-allow-private-ips flag. The new approach informs the user when their provided endpoint URL [would be] valid as a private IP address.
Upon saving a custom model, the URL may be deemed invalid. This change gives a more detailed message to the frontend regarding the endpoint validity, enabling us to present a more helpful error message to the user.
Switching to a model with an invalid endpoint results in an error message being displayed. This change causes the error message to be cleared when switching to another model.
If the user attempts to save a configuration with a private endpoint, and the optional flag has not been enabled, the user will be presented with a modal dialog informing them as much. If the optional flag has been enabled, and the user attempts to use a private endpoint, we will display a label warning them of the risk they're accepting.
Each condition checked `apiHasError`, so we can simplify by moving that to the top-most conditional, and turning the rest of the logic into a switch-case, dropping unnecessary parents around JSX items.
This method no longer queries the backend itself, and therefore no longer needs to be async.
To ease efforts for translators, more descriptive identifiers are provided.
Though not likely to happen, it's possible our method could be called with an invalid number and/or type of arguments. We'll make sure our expectations are clear, and that we reject early otherwise.
Giving proper support to .local domains requires adequate address resolution, which can be somewhat tricky across platforms. For now, we will defer adding support to a later date. Track brave/brave-browser#42367 for additional details and development.
@jonathansampson jonathansampson force-pushed the sampson-issue-39627-private-ip-endpoint branch from f29cbd4 to 7c68536 Compare November 21, 2024 13:57
@jonathansampson jonathansampson enabled auto-merge (squash) November 21, 2024 14:03
@mihaiplesa mihaiplesa merged commit 02ede15 into master Nov 21, 2024
16 of 17 checks passed
@mihaiplesa mihaiplesa deleted the sampson-issue-39627-private-ip-endpoint branch November 21, 2024 17:55
@github-actions github-actions bot added this to the 1.75.x - Nightly milestone Nov 21, 2024
@brave-builds
Copy link
Collaborator

Released in v1.75.43

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/run-upstream-tests Run upstream unit and browser tests on Linux and Windows (otherwise only on Linux) CI/storybook-url Deploy storybook and provide a unique URL for each build enhancement feature/leo Leo-related issues and features feature/privacy Privacy-related feature that doesn't fall under other labels such as feature/shields feature/settings javascript Pull requests that update Javascript code needs-security-review puLL-Merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BYOM: allow private IPs over HTTP with a warning
6 participants