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

fix(platform): avoid agent input value export, handling marketplace redirection, agent store fixes #9147

Merged
merged 12 commits into from
Dec 30, 2024

Conversation

Swiftyos
Copy link
Contributor

@Swiftyos Swiftyos commented Dec 30, 2024

Changes 🏗️

  • Redirect to the marketplace.
  • Ensure that the store agent uses agent graph data instead of store listing data.
  • Don’t export agent input values.
  • URL sanitization: We can’t open an agent if it has a colon in its name.
  • Show all top agents.

Checklist 📋

For code changes:

  • I have clearly listed my changes in the PR description
  • I have made a test plan
  • I have tested my changes according to the test plan:
    • ...
Example test plan
  • Create from scratch and execute an agent with at least 3 blocks
  • Import an agent from file upload, and confirm it executes correctly
  • Upload agent to marketplace
  • Import an agent from marketplace and confirm it executes correctly
  • Edit an agent from monitor, and confirm it executes correctly

For configuration changes:

  • .env.example is updated or already compatible with my changes
  • docker-compose.yml is updated or already compatible with my changes
  • I have included a list of my configuration changes in the PR description (under Changes)
Examples of configuration changes
  • Changing ports
  • Adding new services that need to communicate with each other
  • Secrets or environment variable changes
  • New or infrastructure changes such as databases

@Swiftyos Swiftyos requested a review from a team as a code owner December 30, 2024 13:39
@Swiftyos Swiftyos requested review from ntindle and aarushik93 and removed request for a team December 30, 2024 13:39
@github-actions github-actions bot added platform/frontend AutoGPT Platform - Front end platform/backend AutoGPT Platform - Back end labels Dec 30, 2024
Copy link

netlify bot commented Dec 30, 2024

Deploy Preview for auto-gpt-docs-dev canceled.

Name Link
🔨 Latest commit ba0b0be
🔍 Latest deploy log https://app.netlify.com/sites/auto-gpt-docs-dev/deploys/6772abd9b88d11000818121e

Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 Security concerns

SQL injection:
The search query sanitization in db.py only checks length and strips whitespace, but does not properly escape special SQL characters. This could potentially allow SQL injection attacks if the sanitization in line 38 is not properly implemented.

⚡ Recommended focus areas for review

Error Handling

The new error handling approach returns JSON responses instead of raising exceptions, which could mask underlying issues and make debugging harder. Consider logging the full exception details before returning the generic error responses.

logger.exception("Exception occurred whilst getting user profile")
return fastapi.responses.JSONResponse(
    status_code=500,
    content={"detail": "An error occurred while retrieving the user profile"},
)
Input Validation

The username and slug sanitization logic removes all non-alphanumeric characters except hyphens. This could potentially cause collisions if different usernames/slugs map to the same sanitized value.

slug = "".join(
    c if c.isalpha() or c == "-" or c.isnumeric() else "" for c in slug
).lower()

Copy link

netlify bot commented Dec 30, 2024

Deploy Preview for auto-gpt-docs canceled.

Name Link
🔨 Latest commit ba0b0be
🔍 Latest deploy log https://app.netlify.com/sites/auto-gpt-docs/deploys/6772abd9e72e3e0008d14d8d

@ntindle
Copy link
Member

ntindle commented Dec 30, 2024

That slugifier message above is valid if we allow any other characters so I'd recommend taking a look at it

@ntindle
Copy link
Member

ntindle commented Dec 30, 2024

We could also just generate the slugs we want and check if they already exist rather than requiring the user to pass them in

@Swiftyos
Copy link
Contributor Author

We could also just generate the slugs we want and check if they already exist rather than requiring the user to pass them in

The idea was around SEO, however I see no issue with just auto generating it from the agent name (store listing name).

@Swiftyos
Copy link
Contributor Author

That slugifier message above is valid if we allow any other characters so I'd recommend taking a look at it

The AI hasn't understood the code correctly. The search query is sanitized by having all sql lang special characters escaped.

Also the slug is sanitized prior to saving it, however we are missing a unique constraint on the slug.

Swiftyos and others added 6 commits December 30, 2024 14:55
…ption

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…ption

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
@Swiftyos Swiftyos merged commit 763284e into dev Dec 30, 2024
22 checks passed
@Swiftyos Swiftyos deleted the hotfix-new-year branch December 30, 2024 15:04
@majdyz majdyz changed the title Hotfix new year fix(platform): avoid agent input value export, handling marketplace redirection, agent store fixes Dec 30, 2024
@@ -37,20 +41,24 @@ async def get_profile(
return profile
except Exception:
logger.exception("Exception occurred whilst getting user profile")
raise
return fastapi.responses.JSONResponse(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this actually needed? There is already handle_internal_http_error in rest_api.py, are the v2 routes no longer using it?

@@ -71,15 +79,22 @@ async def update_or_create_profile(
return updated_profile
except Exception:
logger.exception("Exception occurred whilst updating profile")
raise
return fastapi.responses.JSONResponse(
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

@@ -0,0 +1,50 @@
BEGIN;
Copy link
Contributor

Choose a reason for hiding this comment

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

need to have a comment here just like the other migration file

@@ -40,7 +40,8 @@ export default async function Page({
const agent = await api.getStoreAgent(creator_lower, params.slug);
const otherAgents = await api.getStoreAgents({ creator: creator_lower });
const similarAgents = await api.getStoreAgents({
search_query: agent.categories[0],
// We are using slug as we know its has been sanitized and is not null
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: it has been, "and is not null" is inferred on the code too.
Maybe the comment is not needed?

@@ -34,8 +34,8 @@ export const AgentsSection: React.FC<AgentsSectionProps> = ({
}) => {
const router = useRouter();

// Take only the first 9 agents
const displayedAgents = allAgents.slice(0, 9);
// TODO: Update this when we have pagination
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add more clarity? Update this to what?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform/backend AutoGPT Platform - Back end platform/frontend AutoGPT Platform - Front end Review effort [1-5]: 3 size/l
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants