-
Notifications
You must be signed in to change notification settings - Fork 0
Make the design similar to Mozilla's bugzilla #7
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
Conversation
Signed-off-by: Andrew Berezovskyi <andriib@kth.se>
Signed-off-by: Andrew Berezovskyi <andriib@kth.se>
Signed-off-by: Andrew Berezovskyi <andriib@kth.se>
Signed-off-by: Andrew Berezovskyi <andriib@kth.se>
Signed-off-by: Andrew Berezovskyi <andriib@kth.se>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request redesigns the Bugzilla Change Request UI to better match Mozilla's Bugzilla design aesthetic. The changes improve the visual presentation by organizing information into collapsible card-based sections with modern styling, and enhance person data display by extracting and showing actual names from the XML archive instead of placeholder text.
Changes:
- Redesigned the bugzilla change request JSP page with Bootstrap 5 cards, Google Fonts (Fira Sans), and collapsible sections
- Updated Person.toString() to display actual person names (full name, given+family name, or "Unnamed Person") instead of placeholder text
- Enhanced XML parsing to extract person names and email addresses from assigned_to and reporter elements
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| bugzillachangerequest.jsp | Major UI redesign with collapsible card sections, status badges, improved layout, and dynamic property rendering using reflection |
| persontohtml.jsp | Changed to call Person.toString(true) for proper person display |
| Person.java | Implemented toString() logic to display person names intelligently based on available fields |
| BugzillaArchive.java | Added XML parsing for assignedToName and reporterName attributes |
| Bug.java | Added fields for assignedToName, reporterEmail, and reporterName |
| RestDelegate.java | Updated to populate Person objects with extracted names from Bug data |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| String badgeClass = "bg-secondary"; | ||
| if (status.equalsIgnoreCase("NEW") || status.equalsIgnoreCase("UNCONFIRMED")) badgeClass = "bg-primary"; | ||
| else if (status.equalsIgnoreCase("RESOLVED") || status.equalsIgnoreCase("VERIFIED")) badgeClass = "bg-success"; | ||
| else if (status.equalsIgnoreCase("CLOSED")) badgeClass = "bg-dark"; | ||
| else if (status.equalsIgnoreCase("IN_PROGRESS") || status.equalsIgnoreCase("ASSIGNED")) badgeClass = "bg-info text-dark"; | ||
| out.write("<span class=\"badge rounded-pill " + badgeClass + "\">" + status + "</span>"); |
Copilot
AI
Jan 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The status value is used to construct CSS class names without proper validation or sanitization. If the status string contains special characters or unexpected values, it could potentially break the HTML structure or styling. Consider validating the status against a known set of values or sanitizing it before using it in the badge class construction.
| String badgeClass = "bg-secondary"; | |
| if (status.equalsIgnoreCase("NEW") || status.equalsIgnoreCase("UNCONFIRMED")) badgeClass = "bg-primary"; | |
| else if (status.equalsIgnoreCase("RESOLVED") || status.equalsIgnoreCase("VERIFIED")) badgeClass = "bg-success"; | |
| else if (status.equalsIgnoreCase("CLOSED")) badgeClass = "bg-dark"; | |
| else if (status.equalsIgnoreCase("IN_PROGRESS") || status.equalsIgnoreCase("ASSIGNED")) badgeClass = "bg-info text-dark"; | |
| out.write("<span class=\"badge rounded-pill " + badgeClass + "\">" + status + "</span>"); | |
| // HTML-escape the status string to prevent breaking HTML structure or injection | |
| String safeStatus = status | |
| .replace("&", "&") | |
| .replace("<", "<") | |
| .replace(">", ">") | |
| .replace("\"", """) | |
| .replace("'", "'"); | |
| String badgeClass = "bg-secondary"; | |
| if (status.equalsIgnoreCase("NEW") || status.equalsIgnoreCase("UNCONFIRMED")) badgeClass = "bg-primary"; | |
| else if (status.equalsIgnoreCase("RESOLVED") || status.equalsIgnoreCase("VERIFIED")) badgeClass = "bg-success"; | |
| else if (status.equalsIgnoreCase("CLOSED")) badgeClass = "bg-dark"; | |
| else if (status.equalsIgnoreCase("IN_PROGRESS") || status.equalsIgnoreCase("ASSIGNED")) badgeClass = "bg-info text-dark"; | |
| out.write("<span class=\"badge rounded-pill " + badgeClass + "\">" + safeStatus + "</span>"); |
| <% | ||
| Iterator<Person> creatorItr = aBugzillaChangeRequest.getCreator().iterator(); | ||
| while(creatorItr.hasNext()) { | ||
| Person next = creatorItr.next(); | ||
| request.setAttribute("aPerson", next); | ||
| %> | ||
| <jsp:include page="/se/kth/md/it/bcm/persontohtml.jsp"><jsp:param name="asLocalResource" value="true"/></jsp:include> | ||
| <% | ||
| request.removeAttribute("aPerson"); | ||
| if (creatorItr.hasNext()) { out.write(", "); } | ||
| } |
Copilot
AI
Jan 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If getCreator() returns null, calling .iterator() on it will throw a NullPointerException. The code should check if the collection is null before attempting to iterate over it.
| <% | |
| Iterator<Person> creatorItr = aBugzillaChangeRequest.getCreator().iterator(); | |
| while(creatorItr.hasNext()) { | |
| Person next = creatorItr.next(); | |
| request.setAttribute("aPerson", next); | |
| %> | |
| <jsp:include page="/se/kth/md/it/bcm/persontohtml.jsp"><jsp:param name="asLocalResource" value="true"/></jsp:include> | |
| <% | |
| request.removeAttribute("aPerson"); | |
| if (creatorItr.hasNext()) { out.write(", "); } | |
| } | |
| <% | |
| java.util.Collection<Person> creators = aBugzillaChangeRequest.getCreator(); | |
| if (creators == null) { | |
| out.write("<em>null</em>"); | |
| } else { | |
| Iterator<Person> creatorItr = creators.iterator(); | |
| while (creatorItr.hasNext()) { | |
| Person next = creatorItr.next(); | |
| request.setAttribute("aPerson", next); | |
| %> | |
| <jsp:include page="/se/kth/md/it/bcm/persontohtml.jsp"><jsp:param name="asLocalResource" value="true"/></jsp:include> | |
| <% | |
| request.removeAttribute("aPerson"); | |
| if (creatorItr.hasNext()) { out.write(", "); } | |
| } | |
| } |
| if (asLocalResource != null && asLocalResource.equalsIgnoreCase("true")) { | ||
| out.write("{a Local Person Resource} - update Person.toString() to present resource as desired."); | ||
| out.write(aResource.toString(true)); |
Copilot
AI
Jan 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a potential NullPointerException if aResource is null when asLocalResource is true. The code attempts to call aResource.toString(true) without checking if aResource is null first. Consider adding a null check similar to the else branch.
| out.write(aResource.toString(true)); | |
| if (aResource == null) { | |
| out.write("<em>null</em>"); | |
| } else { | |
| out.write(aResource.toString(true)); | |
| } |
| if (name != null) { | ||
| result = name; | ||
| } else if (givenName != null || familyName != null) { | ||
| result = (givenName != null ? givenName : "") + (givenName != null && familyName != null ? " " : "") + (familyName != null ? familyName : ""); |
Copilot
AI
Jan 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The string concatenation logic is complex and hard to read. The condition "(givenName != null && familyName != null ? " " : "")" should be placed between the two names, but currently it's constructed as: givenName + space_condition + familyName. This means if only givenName exists, no space is added (correct), but if only familyName exists, no space is added either (also correct). However, the readability could be improved by simplifying this logic.
| result = (givenName != null ? givenName : "") + (givenName != null && familyName != null ? " " : "") + (familyName != null ? familyName : ""); | |
| if (givenName != null && familyName != null) { | |
| result = givenName + " " + familyName; | |
| } else if (givenName != null) { | |
| result = givenName; | |
| } else { | |
| result = familyName; | |
| } |
| display: inline-block; | ||
| margin-right: 0.5rem; | ||
| font-size: 0.75rem; | ||
| transition: transform 0.2s ease-in-out; | ||
| } | ||
| .card-header[aria-expanded="true"]::before { | ||
| transform: rotate(90deg); | ||
| } | ||
| .card-header[aria-expanded="false"]::before { | ||
| transform: rotate(0deg); | ||
| } | ||
| </style> | ||
| </head> |
Copilot
AI
Jan 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The collapsible card headers use CSS pseudo-elements (::before) to display expand/collapse indicators, but these visual indicators are not accessible to screen readers. Consider adding aria-label or visually hidden text to provide equivalent information to assistive technologies about whether sections are expanded or collapsed.
| <li><a href="<%=type%>"><%=type%></a></li> | ||
| <%}%> | ||
| </ul> | ||
| <h1 class="display-5 mb-4"><%= aBugzillaChangeRequest.toString() %></h1> |
Copilot
AI
Jan 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bug summary composed by aBugzillaChangeRequest.toString() (which uses identifier and title) is written directly into both the <title> element and an <h1> without escaping. If a reporter can craft a bug title containing HTML/JavaScript, this will be reflected verbatim into the page chrome and heading, leading to stored XSS when others view the bug. Ensure the string returned by toString() (or its constituent fields) is HTML-encoded before being inserted into the document title or headings.
| <h1 class="display-5 mb-4"><%= aBugzillaChangeRequest.toString() %></h1> | |
| <% request.setAttribute("bugzillaChangeRequestSummary", aBugzillaChangeRequest.toString()); %> | |
| <h1 class="display-5 mb-4"><c:out value="${bugzillaChangeRequestSummary}" /></h1> |
| <% method = BugzillaChangeRequest.class.getMethod("getProduct"); %> | ||
| <dt class="col-sm-4 text-end"><a href="<%=method.getAnnotation(OslcPropertyDefinition.class).value() %>"><%=method.getAnnotation(OslcName.class).value()%></a></dt> | ||
| <dd class="col-sm-8"> | ||
| <% if (aBugzillaChangeRequest.getProduct() == null) { out.write("<em>null</em>"); } else { out.write(aBugzillaChangeRequest.getProduct().toString()); } %> | ||
| </dd> | ||
| </dl> | ||
| </div> | ||
| <div class="col-md-6"> | ||
| <dl class="row mb-1"> | ||
| <% method = BugzillaChangeRequest.class.getMethod("getComponent"); %> | ||
| <dt class="col-sm-4 text-end"><a href="<%=method.getAnnotation(OslcPropertyDefinition.class).value() %>"><%=method.getAnnotation(OslcName.class).value()%></a></dt> | ||
| <dd class="col-sm-8"> | ||
| <% if (aBugzillaChangeRequest.getComponent() == null) { out.write("<em>null</em>"); } else { out.write(aBugzillaChangeRequest.getComponent().toString()); } %> | ||
| </dd> | ||
| </dl> | ||
| </div> | ||
| <div class="col-md-6"> | ||
| <dl class="row mb-1"> | ||
| <% method = BugzillaChangeRequest.class.getMethod("getVersion"); %> | ||
| <dt class="col-sm-4 text-end"><a href="<%=method.getAnnotation(OslcPropertyDefinition.class).value() %>"><%=method.getAnnotation(OslcName.class).value()%></a></dt> | ||
| <dd class="col-sm-8"> | ||
| <% if (aBugzillaChangeRequest.getVersion() == null) { out.write("<em>null</em>"); } else { out.write(aBugzillaChangeRequest.getVersion().toString()); } %> | ||
| </dd> | ||
| </dl> | ||
| </div> | ||
| <div class="col-md-6"> | ||
| <dl class="row mb-1"> | ||
| <% method = BugzillaChangeRequest.class.getMethod("getPlatform"); %> | ||
| <dt class="col-sm-4 text-end"><a href="<%=method.getAnnotation(OslcPropertyDefinition.class).value() %>"><%=method.getAnnotation(OslcName.class).value()%></a></dt> | ||
| <dd class="col-sm-8"> | ||
| <% if (aBugzillaChangeRequest.getPlatform() == null) { out.write("<em>null</em>"); } else { out.write(aBugzillaChangeRequest.getPlatform().toString()); } %> | ||
| </dd> | ||
| </dl> | ||
| </div> | ||
| <div class="col-md-6"> | ||
| <dl class="row mb-1"> | ||
| <% method = BugzillaChangeRequest.class.getMethod("getOperatingSystem"); %> | ||
| <dt class="col-sm-4 text-end"><a href="<%=method.getAnnotation(OslcPropertyDefinition.class).value() %>"><%=method.getAnnotation(OslcName.class).value()%></a></dt> | ||
| <dd class="col-sm-8"> | ||
| <% if (aBugzillaChangeRequest.getOperatingSystem() == null) { out.write("<em>null</em>"); } else { out.write(aBugzillaChangeRequest.getOperatingSystem().toString()); } %> | ||
| </dd> | ||
| </dl> | ||
| </div> | ||
| <div class="col-md-6"> | ||
| <dl class="row mb-1"> | ||
| <% method = BugzillaChangeRequest.class.getMethod("getPriority"); %> | ||
| <dt class="col-sm-4 text-end"><a href="<%=method.getAnnotation(OslcPropertyDefinition.class).value() %>"><%=method.getAnnotation(OslcName.class).value()%></a></dt> | ||
| <dd class="col-sm-8"> | ||
| <% if (aBugzillaChangeRequest.getPriority() == null) { out.write("<em>null</em>"); } else { out.write(aBugzillaChangeRequest.getPriority().toString()); } %> | ||
| </dd> | ||
| </dl> | ||
| </div> | ||
| <div class="col-md-6"> | ||
| <dl class="row mb-1"> | ||
| <% method = BugzillaChangeRequest.class.getMethod("getDctermsType"); %> | ||
| <dt class="col-sm-4 text-end"><a href="<%=method.getAnnotation(OslcPropertyDefinition.class).value() %>"><%=method.getAnnotation(OslcName.class).value()%></a></dt> | ||
| <dd class="col-sm-8"> | ||
| <ul> | ||
| <% for(Object next : aBugzillaChangeRequest.getDctermsType()) { %> | ||
| <li><jsp:include page="/se/kth/md/it/bcm/typetohtml.jsp"><jsp:param name="asLocalResource" value="true"/></jsp:include></li> | ||
| <% } %> | ||
| </ul> | ||
| </dd> | ||
| </dl> | ||
| </div> | ||
| <div class="col-md-6"> | ||
| <dl class="row mb-1"> | ||
| <% method = BugzillaChangeRequest.class.getMethod("getSubject"); %> | ||
| <dt class="col-sm-4 text-end"><a href="<%=method.getAnnotation(OslcPropertyDefinition.class).value() %>"><%=method.getAnnotation(OslcName.class).value()%></a></dt> | ||
| <dd class="col-sm-8"> | ||
| <ul> | ||
| <% Iterator<String> subjectItr = aBugzillaChangeRequest.getSubject().iterator(); while(subjectItr.hasNext()) { out.write("<li>" + subjectItr.next().toString() + "</li>"); } %> | ||
| </ul> | ||
| </dd> | ||
| </dl> |
Copilot
AI
Jan 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "Categories" section renders multiple string fields from the change request (product, component, version, platform, operatingSystem, priority, and each subject entry) directly into the HTML using out.write(...) and string concatenation without HTML escaping. Since these attributes can be influenced by bug reporters or external systems, an attacker could inject HTML/JavaScript that is stored in Bugzilla and executed when this view is rendered. Apply HTML encoding (or use escaping JSP taglibs) for all these string properties before output so that category metadata cannot be interpreted as markup.
| { | ||
| QName key = entry.getKey(); | ||
| Object value = entry.getValue(); | ||
| %> | ||
| <div class="col-md-6"> | ||
| <dl class="row mb-1"> | ||
| <dt class="col-sm-4 text-end"><a href="<%=key.getNamespaceURI() + key.getLocalPart() %>"><%=key.getLocalPart()%></a></dt> | ||
| <dd class="col-sm-8"><%= value.toString()%></dd> | ||
| </dl> | ||
| </div> | ||
| <% |
Copilot
AI
Jan 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Extended Properties card writes each extended property value with <%= value.toString() %> directly into the HTML without any escaping. Extended properties can originate from external OSLC clients and may contain attacker-controlled text, so a crafted value with HTML/JavaScript would be stored and then executed when this page is viewed (stored XSS). HTML-encode value.toString() (or use an escaping-aware tag) before rendering so that arbitrary extension data cannot break out of the text context.
| if (asLocalResource != null && asLocalResource.equalsIgnoreCase("true")) { | ||
| out.write("{a Local Person Resource} - update Person.toString() to present resource as desired."); | ||
| out.write(aResource.toString(true)); |
Copilot
AI
Jan 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Person name is written directly into the HTML with out.write(aResource.toString(true)) without any HTML escaping. If an attacker can control Person.name, givenName, or familyName (for example via an external user directory or profile data), they can inject HTML/JavaScript that will execute in the browser whenever this page renders the person as a local resource. Ensure Person fields are HTML-encoded before output (or use a tag that escapes by default) so that user-controlled names cannot break out of the text context.
| <dd class="col-sm-6"> | ||
| <% if (aBugzillaChangeRequest.getIdentifier() == null) { out.write("<em>null</em>"); } else { out.write(aBugzillaChangeRequest.getIdentifier().toString()); } %> | ||
| </dd> | ||
| </dl> | ||
| </div> | ||
| <div class="col-md-8"> | ||
| <dl class="row mb-1"> | ||
| <% method = BugzillaChangeRequest.class.getMethod("getTitle"); %> | ||
| <dt class="col-sm-3 text-end"><a href="<%=method.getAnnotation(OslcPropertyDefinition.class).value() %>"><%=method.getAnnotation(OslcName.class).value()%></a></dt> | ||
| <dd class="col-sm-9"> | ||
| <% if (aBugzillaChangeRequest.getTitle() == null) { out.write("<em>null</em>"); } else { out.write(aBugzillaChangeRequest.getTitle().toString()); } %> | ||
| </dd> | ||
| </dl> | ||
| </div> | ||
| <div class="col-md-4"> | ||
| <dl class="row mb-1"> | ||
| <% method = BugzillaChangeRequest.class.getMethod("getStatus"); %> | ||
| <dt class="col-sm-6 text-end"><a href="<%=method.getAnnotation(OslcPropertyDefinition.class).value() %>"><%=method.getAnnotation(OslcName.class).value()%></a></dt> | ||
| <dd class="col-sm-6"> | ||
| <% | ||
| if (aBugzillaChangeRequest.getStatus() == null) { | ||
| out.write("<em>null</em>"); | ||
| } else { | ||
| String status = aBugzillaChangeRequest.getStatus().toString(); | ||
| String badgeClass = "bg-secondary"; | ||
| if (status.equalsIgnoreCase("NEW") || status.equalsIgnoreCase("UNCONFIRMED")) badgeClass = "bg-primary"; | ||
| else if (status.equalsIgnoreCase("RESOLVED") || status.equalsIgnoreCase("VERIFIED")) badgeClass = "bg-success"; | ||
| else if (status.equalsIgnoreCase("CLOSED")) badgeClass = "bg-dark"; | ||
| else if (status.equalsIgnoreCase("IN_PROGRESS") || status.equalsIgnoreCase("ASSIGNED")) badgeClass = "bg-info text-dark"; | ||
| out.write("<span class=\"badge rounded-pill " + badgeClass + "\">" + status + "</span>"); | ||
| } | ||
| %> | ||
| </dd> | ||
| </dl> | ||
| </div> | ||
| <div class="col-md-8"> | ||
| <dl class="row mb-1"> | ||
| <% method = BugzillaChangeRequest.class.getMethod("getShortTitle"); %> | ||
| <dt class="col-sm-3 text-end"><a href="<%=method.getAnnotation(OslcPropertyDefinition.class).value() %>"><%=method.getAnnotation(OslcName.class).value()%></a></dt> | ||
| <dd class="col-sm-9"> | ||
| <% if (aBugzillaChangeRequest.getShortTitle() == null) { out.write("<em>null</em>"); } else { out.write(aBugzillaChangeRequest.getShortTitle().toString()); } %> | ||
| </dd> | ||
| </dl> | ||
| </div> | ||
| <div class="col-md-12"> | ||
| <dl class="row mb-1"> | ||
| <% method = BugzillaChangeRequest.class.getMethod("getDescription"); %> | ||
| <dt class="col-sm-2 text-end"><a href="<%=method.getAnnotation(OslcPropertyDefinition.class).value() %>"><%=method.getAnnotation(OslcName.class).value()%></a></dt> | ||
| <dd class="col-sm-10"> | ||
| <% if (aBugzillaChangeRequest.getDescription() == null) { out.write("<em>null</em>"); } else { out.write(aBugzillaChangeRequest.getDescription().toString()); } %> | ||
| </dd> | ||
| </dl> | ||
| </div> |
Copilot
AI
Jan 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Several bug fields (identifier, title, shortTitle, and description) are rendered with out.write(...)/<%= directly into the HTML body without any escaping (e.g., lines rendering getIdentifier(), getTitle(), getShortTitle(), and getDescription()). Because these values come from the Bugzilla change request and are ultimately controlled by bug reporters, a malicious value containing HTML/JavaScript will be stored and then executed in the browser of anyone viewing this page (stored XSS). Use HTML-encoding for these fields (or JSP tags that escape by default) before output so that user-provided bug text is treated as data, not markup.
No description provided.