Skip to content

Fix/google form response contract #617

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.

Sign up for GitHub

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

Open
Ashvin-KS wants to merge 2 commits into
base: main
Choose a base branch
from
+70 −28
Open

Fix/google form response contract #617

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
515e7cd
fix(web): guard Output page against undefined forEach payloads
Ashvin-KS Mar 19, 2026
1dbfb4b
fix: normalize /generate_gform response contract across clients
Ashvin-KS Mar 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 9 additions & 8 deletions backend/server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
from string import punctuation
from heapq import nlargest
import random
import webbrowser
from apiclient import discovery
from httplib2 import Http
from oauth2client import client, file, tools
Expand Down Expand Up @@ -263,9 +262,13 @@ def get_content():

@app.route("/generate_gform", methods=["POST"])
def generate_gform():
data = request.get_json()
qa_pairs = data.get("qa_pairs", "")
data = request.get_json() or {}
qa_pairs = data.get("qa_pairs", [])
question_type = data.get("question_type", "")
Comment on lines +265 to 267
Copy link

@coderabbitai coderabbitai bot Mar 21, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Validate JSON root type before calling .get().

At Line 265–266, a JSON array/string payload will make data.get(...) fail with 500 instead of returning a clear 400. Add an object-shape check first.

Proposed fix 
-    data = request.get_json() or {}
+    data = request.get_json(silent=True) or {}
+    if not isinstance(data, dict):
+        return jsonify({"error": "Request body must be a JSON object"}), 400
     qa_pairs = data.get("qa_pairs", [])
📝 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.

Suggested change
data = request.get_json() or {}
qa_pairs = data.get("qa_pairs", [])
question_type = data.get("question_type", "")
data = request.get_json(silent=True) or {}
if not isinstance(data, dict):
return jsonify({"error": "Request body must be a JSON object"}), 400
qa_pairs = data.get("qa_pairs", [])
question_type = data.get("question_type", "")
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@backend/server.py` around lines 265 - 267, The handler currently assumes
request.get_json() returns an object and directly calls data.get(...), which
fails for JSON arrays/strings; validate that the parsed JSON (the variable data)
is a dict before using .get. Update the route handler around the
request.get_json() call to check isinstance(data, dict) (or equivalent) and
return a clear 400/BadRequest when it is not an object, then safely access
qa_pairs and question_type only after that check.


if not isinstance(qa_pairs, list):
return jsonify({"error": "qa_pairs must be a list"}), 400

Comment on lines +265 to +271
Copy link

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

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

There’s now new behavior to validate payload shape (qa_pairs must be a list) and return a 400. Since the repo has endpoint-level tests in backend/test_server.py, please add coverage for this branch (at least the 400 on non-list qa_pairs, which avoids any external Google API calls).

Copilot uses AI. Check for mistakes.
SCOPES = "https://www.googleapis.com/auth/forms.body"
DISCOVERY_DOC = "https://forms.googleapis.com/$discovery/rest?version=v1"

Expand Down Expand Up @@ -423,11 +426,9 @@ def generate_gform():
formId=result["formId"], body=NEW_QUESTION
).execute()

edit_url = jsonify(result["responderUri"])
webbrowser.open_new_tab(
"https://docs.google.com/forms/d/" + result["formId"] + "/edit"
)
return edit_url
form_link = result.get("responderUri")
edit_link = f"https://docs.google.com/forms/d/{result['formId']}/edit"
return jsonify({"form_link": form_link, "edit_link": edit_link})


@app.route("/get_shortq_hard", methods=["POST"])
Expand Down
61 changes: 43 additions & 18 deletions eduaid_web/src/pages/Output.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,25 +91,42 @@ const Output = () => {
};

useEffect(() => {
const qaPairsFromStorage =
JSON.parse(localStorage.getItem("qaPairs")) || {};
let qaPairsFromStorage = {};
try {
qaPairsFromStorage = JSON.parse(localStorage.getItem("qaPairs")) || {};
} catch (error) {
console.error("Invalid qaPairs in localStorage", error);
qaPairsFromStorage = {};
}

if (qaPairsFromStorage) {
const combinedQaPairs = [];
const output = Array.isArray(qaPairsFromStorage["output"])
? qaPairsFromStorage["output"]
: [];
const outputMcqQuestions = Array.isArray(
qaPairsFromStorage["output_mcq"]?.["questions"]
)
? qaPairsFromStorage["output_mcq"]["questions"]
: [];
const outputBoolQuestions = Array.isArray(
qaPairsFromStorage["output_boolq"]?.["Boolean_Questions"]
)
? qaPairsFromStorage["output_boolq"]["Boolean_Questions"]
: [];

if (qaPairsFromStorage["output_boolq"]) {
qaPairsFromStorage["output_boolq"]["Boolean_Questions"].forEach(
(question, index) => {
combinedQaPairs.push({
question,
question_type: "Boolean",
context: qaPairsFromStorage["output_boolq"]["Text"],
});
}
);
outputBoolQuestions.forEach((question) => {
combinedQaPairs.push({
question,
question_type: "Boolean",
context: qaPairsFromStorage["output_boolq"]["Text"],
});
});
}

if (qaPairsFromStorage["output_mcq"]) {
qaPairsFromStorage["output_mcq"]["questions"].forEach((qaPair) => {
outputMcqQuestions.forEach((qaPair) => {
combinedQaPairs.push({
question: qaPair.question_statement,
question_type: "MCQ",
Expand All @@ -121,7 +138,7 @@ const Output = () => {
}

if (qaPairsFromStorage["output_mcq"] || questionType === "get_mcq") {
qaPairsFromStorage["output"].forEach((qaPair) => {
output.forEach((qaPair) => {
combinedQaPairs.push({
question: qaPair.question_statement,
question_type: "MCQ",
Expand All @@ -132,15 +149,15 @@ const Output = () => {
});
}

if (questionType == "get_boolq") {
qaPairsFromStorage["output"].forEach((qaPair) => {
if (questionType === "get_boolq") {
output.forEach((qaPair) => {
combinedQaPairs.push({
question: qaPair,
question_type: "Boolean",
});
});
} else if (qaPairsFromStorage["output"] && questionType !== "get_mcq") {
qaPairsFromStorage["output"].forEach((qaPair) => {
output.forEach((qaPair) => {
combinedQaPairs.push({
question:
qaPair.question || qaPair.question_statement || qaPair.Question,
Expand All @@ -154,15 +171,23 @@ const Output = () => {

setQaPairs(combinedQaPairs);
}
}, []);
}, [questionType]);

const generateGoogleForm = async () => {
try {
const result = await apiClient.post("/generate_gform", {
qa_pairs: qaPairs,
question_type: questionType,
});
const formUrl = result.form_link;
const formUrl =
(result && result.form_link) ||
(typeof result === "string" ? result : null);

if (!formUrl) {
console.error("Google Form URL missing in API response", result);
return;
}

Comment on lines +182 to +190
Copy link

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

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

This response parsing pattern is now duplicated in multiple clients. Consider extracting it into a small utility (web-side) so the /generate_gform response contract is handled consistently and changes are easier to roll out.

Copilot uses AI. Check for mistakes.
window.open(formUrl, "_blank");
Copy link

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

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

Opening a URL in a new tab with target=_blank should include noopener/noreferrer (or opener = null) to avoid reverse-tabnabbing. This is especially relevant since the URL comes from an API response.

Suggested change
window.open(formUrl, "_blank");
const newWindow = window.open(formUrl, "_blank", "noopener,noreferrer");
if (newWindow) {
newWindow.opener = null;
}

Copilot uses AI. Check for mistakes.
} catch (error) {
console.error("Failed to generate Google Form:", error);
Expand Down
10 changes: 9 additions & 1 deletion extension/src/pages/question/Question.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,15 @@ function Question() {

if (response.ok) {
const result = await response.json();
const formUrl = result.form_link;
const formUrl =
(result && result.form_link) ||
(typeof result === "string" ? result : null);

if (!formUrl) {
console.error("Google Form URL missing in API response", result);
return;
}
Comment on lines +115 to +122
Copy link

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

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

The legacy/new response handling is repeated across multiple components. Consider factoring this into a shared helper so any future API contract adjustments (e.g., preferring edit_link in some flows) only need to be made once.

Copilot uses AI. Check for mistakes.

window.open(formUrl, "_blank");
Copy link

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

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

window.open(..., "_blank") should be opened with noopener/noreferrer (or newWindow.opener = null) to prevent reverse-tabnabbing from a potentially attacker-controlled URL.

Suggested change
window.open(formUrl, "_blank");
const newWindow = window.open(formUrl, "_blank", "noopener,noreferrer");
if (newWindow) {
newWindow.opener = null;
}

Copilot uses AI. Check for mistakes.
} else {
console.error("Failed to generate Google Form");
Expand Down
10 changes: 9 additions & 1 deletion extension/src/pages/question/SidePanel.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,15 @@ function SidePanel() {

if (response.ok) {
const result = await response.json();
const formUrl = result.form_link;
const formUrl =
(result && result.form_link) ||
(typeof result === "string" ? result : null);

if (!formUrl) {
console.error("Google Form URL missing in API response", result);
return;
}
Comment on lines +118 to +125
Copy link

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

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

The response-shape normalization logic is duplicated here (and in other clients). To avoid the extension/web drifting on future contract tweaks (e.g., adding edit_link or changing legacy handling), consider extracting a small helper (e.g., getGFormLinkFromResponse(result)) and reusing it across the three call sites.

Copilot uses AI. Check for mistakes.

window.open(formUrl, "_blank");
Copy link

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

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

window.open(formUrl, "_blank") can allow reverse-tabnabbing via window.opener. Prefer opening with noopener,noreferrer (or explicitly nulling opener) so the newly opened page can't navigate the extension page.

Suggested change
window.open(formUrl, "_blank");
const newWindow = window.open(formUrl, "_blank", "noopener,noreferrer");
if (newWindow) {
newWindow.opener = null;
}

Copilot uses AI. Check for mistakes.
} else {
console.error("Failed to generate Google Form");
Expand Down
Loading