mraible commented on code in PR #162:
URL: https://github.com/apache/roller/pull/162#discussion_r3891402036
##########
app/src/main/java/org/apache/roller/weblogger/ui/struts2/editor/MediaFileBase.java:
##########
@@ -62,7 +62,7 @@ protected void doDeleteMediaFile() {
log.debug("Processing delete of file id - " + this.mediaFileId);
MediaFileManager manager = WebloggerFactory.getWeblogger()
.getMediaFileManager();
- MediaFile mediaFile = manager.getMediaFile(this.mediaFileId);
+ MediaFile mediaFile = manager.getMediaFile(getActionWeblog(),
this.mediaFileId);
Review Comment:
doDeleteMediaFile here, doIncludeMediaFileInGallery (line 87), and
doMoveSelected (line 150, targetDirectory) all pass the now-nullable scoped
result straight through: removeMediaFile(weblog, null) ends in an
IllegalArgumentException from em.remove(null), the others NPE, and the catch
only covers WebloggerException. A foreign or stale id now yields a 500 page
instead of the mediaFile.delete.error / includeInGallery.error / move.errors
messages the code intends.
##########
app/src/main/java/org/apache/roller/weblogger/ui/struts2/editor/Bookmarks.java:
##########
@@ -225,12 +225,12 @@ public String move() {
}
// Move bookmarks to new parent folder.
- WeblogBookmarkFolder newFolder =
bmgr.getFolder(getTargetFolderId());
+ WeblogBookmarkFolder newFolder =
bmgr.getFolderById(getActionWeblog(), getTargetFolderId());
String bookmarks[] = getSelectedBookmarks();
if (null != bookmarks && bookmarks.length > 0) {
for (int j = 0; j < bookmarks.length; j++) {
- WeblogBookmark bd = bmgr.getBookmark(bookmarks[j]);
+ WeblogBookmark bd = bmgr.getBookmark(getActionWeblog(),
bookmarks[j]);
newFolder.addBookmark(bd);
Review Comment:
move() never null-checks newFolder (line 228) or bd (line 233), although
delete() at line 138 does. A targetFolderId from another weblog gives newFolder
== null, addBookmark NPEs past the WebloggerException catch, and the user gets
a 500 rather than bookmarksForm.error.move. An unresolved selectedBookmarks
entry also adds a null to the folder's in-memory list before bd.setFolder blows
up.
##########
app/src/main/java/org/apache/roller/weblogger/ui/struts2/editor/EntryEdit.java:
##########
@@ -104,7 +104,7 @@ public void myPrepare() {
// retrieve from DB WeblogEntry based on ID
WeblogEntryManager wmgr = WebloggerFactory.getWeblogger()
.getWeblogEntryManager();
- setEntry(wmgr.getWeblogEntry(getBean().getId()));
+ setEntry(wmgr.getWeblogEntry(getActionWeblog(),
getBean().getId()));
} catch (WebloggerException ex) {
log.error(
"Error looking up entry by id - " + getBean().getId(),
Review Comment:
myPrepare now leaves entry null for any id outside the action weblog (the
new test asserts exactly that), but execute() (line 126) hands it to
EntryBean.copyFrom, which calls entry.getId() immediately. A co-author deleting
an entry you have open, or a crafted bean.id, is now a stack-trace page rather
than a not-found message. TemplateEdit already handles the same null with
'Unable to locate specified template'; CategoryEdit.execute,
FolderEdit.myValidate, and BookmarkEdit.myValidate (line 123) have the same gap.
##########
app/src/main/java/org/apache/roller/weblogger/ui/struts2/editor/MediaFileEdit.java:
##########
@@ -67,7 +67,7 @@ public void myPrepare() {
try {
MediaFileManager mgr =
WebloggerFactory.getWeblogger().getMediaFileManager();
if (!StringUtils.isEmpty(bean.getDirectoryId())) {
- setDirectory(mgr.getMediaFileDirectory(bean.getDirectoryId()));
+ setDirectory(mgr.getMediaFileDirectory(getActionWeblog(),
bean.getDirectoryId()));
}
} catch (WebloggerException ex) {
log.error("Error looking up media file directory", ex);
Review Comment:
The directory set here can now be null for a directoryId outside the action
weblog, and myValidate (line 82) calls getDirectory().getMediaFile(...) on it.
save() runs myValidate before entering its try, so a foreign or deleted
directory id is an unhandled NPE instead of INPUT with a validation error.
##########
app/src/main/java/org/apache/roller/weblogger/ui/struts2/editor/MediaFileImageDim.java:
##########
@@ -50,7 +50,7 @@ public MediaFileImageDim() {
public String execute() {
try {
MediaFileManager mgr =
WebloggerFactory.getWeblogger().getMediaFileManager();
- MediaFile mediaFile = mgr.getMediaFile(getMediaFileId());
+ MediaFile mediaFile = mgr.getMediaFile(getActionWeblog(),
getMediaFileId());
bean.copyFrom(mediaFile);
Review Comment:
Same family: the scoped lookup can return null and bean.copyFrom(mediaFile)
dereferences it inside a catch that only handles WebloggerException.
##########
app/src/main/java/org/apache/roller/weblogger/ui/struts2/editor/Comments.java:
##########
@@ -102,7 +102,7 @@ public void loadComments() {
// lookup weblog entry if necessary
if (!StringUtils.isEmpty(getBean().getEntryId())) {
- setQueryEntry(wmgr.getWeblogEntry(getBean().getEntryId()));
+ setQueryEntry(wmgr.getWeblogEntry(getActionWeblog(),
getBean().getEntryId()));
Review Comment:
Different failure shape here: a foreign or stale entryId now makes
queryEntry null, and null silently means 'all comments in the weblog', so the
listing widens from one entry to the whole blog without any indication.
Probably worth treating an unresolved entryId as an empty result or an error
rather than as no filter.
##########
app/src/main/java/org/apache/roller/weblogger/ui/struts2/editor/CategoryRemove.java:
##########
@@ -112,7 +112,7 @@ public String remove() {
WeblogEntryManager wmgr =
WebloggerFactory.getWeblogger().getWeblogEntryManager();
if (getTargetCategoryId() != null) {
- WeblogCategory target =
wmgr.getWeblogCategory(getTargetCategoryId());
+ WeblogCategory target =
wmgr.getWeblogCategory(getActionWeblog(), getTargetCategoryId());
wmgr.moveWeblogCategoryContents(getCategory(), target);
Review Comment:
targetCategoryId outside the action weblog gives a null target here, which
goes straight into moveWeblogCategoryContents.
##########
app/src/main/java/org/apache/roller/weblogger/ui/struts2/editor/EntryAddWithMediaFile.java:
##########
@@ -71,7 +71,7 @@ public String execute() {
if (selectedImages != null) {
for (String image : selectedImages) {
- MediaFile mediaFile = manager.getMediaFile(image);
+ MediaFile mediaFile =
manager.getMediaFile(getActionWeblog(), image);
String link;
if (mediaFile.isImageFile()) {
Review Comment:
A selected image outside the action weblog now resolves to null and the loop
dereferences it; the generic catch swallows the NPE, so the new-entry form
silently loses the media links rather than telling the user what happened.
##########
app/src/main/java/org/apache/roller/weblogger/ui/struts2/editor/WeblogConfig.java:
##########
@@ -136,7 +136,7 @@ public String save() {
// if blogger category changed then lookup new cat and set it
if(getBean().getBloggerCategoryId() != null &&
!weblog.getBloggerCategory().getId().equals(getBean().getBloggerCategoryId())) {
-
weblog.setBloggerCategory(wmgr.getWeblogCategory(getBean().getBloggerCategoryId()));
+
weblog.setBloggerCategory(wmgr.getWeblogCategory(getActionWeblog(),
getBean().getBloggerCategoryId()));
Review Comment:
This one persists bad data: when bloggerCategoryId doesn't resolve inside
the action weblog, the scoped lookup returns null and setBloggerCategory(null)
is committed (bloggercatid is nullable). From then on line 138 dereferences
weblog.getBloggerCategory() on every save, inside a catch that only handles
WebloggerException, so the Settings page for that weblog 500s until someone
repairs the row. A null check that adds a validation error and leaves the
existing category alone closes it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
