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.

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

Added and improved user input validation for LSPTemplateUI #7893

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+44 −11
Open

Added and improved user input validation for LSPTemplateUI #7893

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
55 changes: 44 additions & 11 deletions java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/protocol/LspTemplateUI.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import java.util.logging.Level;
import java.util.logging.Logger;
import java.util.stream.Collectors;
import javax.lang.model.SourceVersion;
import org.eclipse.lsp4j.ExecuteCommandParams;
import org.eclipse.lsp4j.MessageParams;
import org.eclipse.lsp4j.MessageType;
Expand Down Expand Up @@ -78,9 +79,16 @@
"CTL_TemplateUI_SelectPackageNameSuggestion=org.yourcompany.yourproject",
"CTL_TemplateUI_SelectName=Name of the object?",
"# {0} - path",
"ERR_InvalidPath={0} isn't valid folder",
"ERR_InvalidPath={0} isn't valid folder or is read only",
"# {0} - path",
"ERR_ExistingPath={0} already exists",
"# {0} - packageName",
"ERR_InvalidPackageName={0} isn't valid package name",
"# {0} - path",
"ERR_InvalidNewPath={0} isn't valid path or is read only",
"# {0} - ObjectName",
"ERR_InvalidObjectName={0} isn't valid object name"

})
final class LspTemplateUI {
/**
Expand Down Expand Up @@ -165,8 +173,20 @@ private CompletableFuture<Object> projectUI(DataFolder templates, NbCodeLanguage
}

private static CompletionStage<String> findPackage(CompletionStage<?> uiBefore, NbCodeLanguageClient client) {
return uiBefore.thenCompose((__) ->
client.showInputBox(new ShowInputBoxParams(Bundle.CTL_TemplateUI_SelectPackageName(), Bundle.CTL_TemplateUI_SelectPackageNameSuggestion()))
return uiBefore.thenCompose((__) -> {
class ValidatePackageName implements Function<String, CompletionStage<String>> {

@Override
public CompletionStage<String> apply(String packageName) {
if (!SourceVersion.isName(packageName)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Here, we are specifically looking for a package name, so the check makes sense to me.

client.showMessage(new MessageParams(MessageType.Error, Bundle.ERR_InvalidPackageName(packageName)));
return client.showInputBox(new ShowInputBoxParams(Bundle.CTL_TemplateUI_SelectPackageName(), Bundle.CTL_TemplateUI_SelectPackageNameSuggestion())).thenCompose(this);
}
return CompletableFuture.completedFuture(packageName);
}
}
return client.showInputBox(new ShowInputBoxParams(Bundle.CTL_TemplateUI_SelectPackageName(), Bundle.CTL_TemplateUI_SelectPackageNameSuggestion())).thenCompose(new ValidatePackageName());
}
);
}

Expand All @@ -188,8 +208,9 @@ public CompletionStage<DataFolder> apply(String path) {
if (path == null) {
throw raise(RuntimeException.class, new UserCancelException(path));
}
FileObject fo = FileUtil.toFileObject(new File(path));
if (fo == null || !fo.isFolder()) {
File target = new File(path);
FileObject fo = FileUtil.toFileObject(target);
if (!target.canWrite() || fo == null || !fo.isFolder()) {
client.showMessage(new MessageParams(MessageType.Error, Bundle.ERR_InvalidPath(path)));
return client.showInputBox(new ShowInputBoxParams(Bundle.CTL_TemplateUI_SelectTarget(), suggestion.getPrimaryFile().getPath())).thenCompose(this);
}
Expand All @@ -202,7 +223,7 @@ public CompletionStage<DataFolder> apply(String path) {

private static CompletionStage<Pair<DataFolder, String>> findTargetAndNameForProject(CompletionStage<DataObject> findTemplate, NbCodeLanguageClient client) {
return findTemplate.thenCompose(__ -> client.workspaceFolders()).thenCompose(folders -> {
class VerifyNonExistingFolder implements Function<String, CompletionStage<Pair<DataFolder,String>>> {
class VerifyNewFolderCreation implements Function<String, CompletionStage<Pair<DataFolder,String>>> {
@Override
public CompletionStage<Pair<DataFolder,String>> apply(String path) {
if (path == null) {
Expand All @@ -215,12 +236,14 @@ public CompletionStage<Pair<DataFolder,String>> apply(String path) {
}
targetPath.getParentFile().mkdirs();
FileObject fo = FileUtil.toFileObject(targetPath.getParentFile());
if (fo == null || !fo.isFolder()) {
if (fo == null || !fo.isFolder() || !targetPath.getParentFile().canWrite() || !SourceVersion.isName(targetPath.getName())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not quite sure I read the patch correctly, but this is the folder where the project will be created. And the name of this folder does not necessarily need to follow the Java name rules(?). I mean, I can have a Java project in a directory called my-current-java-project, and there's no problem in that? Or do I miss something here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right it will be the name of the folder where the project will be created . We need the java name rules because that is also used for the name of the main class . So if the path is /Users/Projects/my-current-java-project it will try to create a project with the main class name as my-current-java-project.

Copy link
Member

Choose a reason for hiding this comment

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

this still checks java constraints for non-java projects.

client.showMessage(new MessageParams(MessageType.Error, Bundle.ERR_InvalidNewPath(path)));
return client.showInputBox(new ShowInputBoxParams(Bundle.CTL_TemplateUI_SelectProjectTarget(), suggestWorkspaceRoot(folders))).thenCompose(this);
}
return CompletableFuture.completedFuture(Pair.of(DataFolder.findFolder(fo), targetPath.getName()));
}
}
return client.showInputBox(new ShowInputBoxParams(Bundle.CTL_TemplateUI_SelectProjectTarget(), suggestWorkspaceRoot(folders))).thenCompose(new VerifyNonExistingFolder());
return client.showInputBox(new ShowInputBoxParams(Bundle.CTL_TemplateUI_SelectProjectTarget(), suggestWorkspaceRoot(folders))).thenCompose(new VerifyNewFolderCreation());
});
}

Expand All @@ -229,9 +252,19 @@ private static CompletionStage<FileBuilder> configure(FileBuilder builder, NbCod
FileObject template = desc.getTemplate();
Object handler = template.getAttribute(FileBuilder.ATTR_TEMPLATE_HANDLER);
if (handler == null) {
return client.showInputBox(new ShowInputBoxParams(Bundle.CTL_TemplateUI_SelectName(), desc.getProposedName())).thenApply(name -> {
return name != null ? builder.name(name) : null;
});
class ValidateJavaObjectName implements Function<String, CompletionStage<String>> {

@Override
public CompletionStage<String> apply(String name) {
if (!SourceVersion.isName(name)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This may be OK for Java templates, but probably not for others (like plain files, XML/HTML files, etc.) And this will be used for all file types, I think. I don't know offhand how to do validation only for Java, it might need another attribute on the template(?). @sdedic? Do I miss something?

Copy link
Member

Choose a reason for hiding this comment

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

I don't know offhand how to do validation only for Java, it might need another attribute on the template(?). @sdedic? Do I miss something?

I think a validator could specified by a template's file attribute, or by template's MIME type (for singe-file templates).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lahodaj Thanks for pointing that out ! I looked up a bit more on this , it seems that all java templates have names ending with ".java" suffix , Can you confirm that ? If thats the case then I can condition on the template name whether to apply the validation check or not .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the PR with conditional validation using MIME type .

client.showMessage(new MessageParams(MessageType.Error, Bundle.ERR_InvalidObjectName(name)));
return client.showInputBox(new ShowInputBoxParams(Bundle.CTL_TemplateUI_SelectName(), desc.getProposedName())).thenCompose(this);
}
return CompletableFuture.completedFuture(name);
}
}
boolean isJavaTemplate = FileUtil.getMIMEType(template)!=null ? FileUtil.getMIMEType(template).equals("text/x-java"):false;
return isJavaTemplate ? client.showInputBox(new ShowInputBoxParams(Bundle.CTL_TemplateUI_SelectName(), desc.getProposedName())).thenCompose(new ValidateJavaObjectName()).thenApply(name -> builder.name(name)) : client.showInputBox(new ShowInputBoxParams(Bundle.CTL_TemplateUI_SelectName(), desc.getProposedName())).thenApply(name -> {return name != null ? builder.name(name) : null;});
}
return CompletableFuture.completedFuture(builder);
}
Expand Down